[libvirt] [PATCHv3 0/6] flags cleanup conclusion
With this, I think I've addressed all the review comments from v2: https://www.redhat.com/archives/libvir-list/2011-July/msg00460.html as well as a side discussion on public API flag filtering: https://www.redhat.com/archives/libvir-list/2011-July/msg00833.html Eric Blake (6): flags: use common dumpxml flags check flags: simplify qemu migration flags esx: reject unknown flags xen: reject unknown flags libvirt: do not mix internal flags into public API build: add syntax check for proper flags use cfg.mk | 30 ++++++++++-- src/conf/domain_conf.c | 25 +++++++-- src/conf/domain_conf.h | 10 ---- src/driver.c | 4 -- src/driver.h | 13 ++--- src/esx/esx_device_monitor.c | 4 +- src/esx/esx_driver.c | 35 ++++++++++--- src/esx/esx_interface_driver.c | 4 +- src/esx/esx_network_driver.c | 4 +- src/esx/esx_nwfilter_driver.c | 4 +- src/esx/esx_secret_driver.c | 4 +- src/esx/esx_storage_driver.c | 4 +- src/fdstream.c | 28 +++++----- src/fdstream.h | 6 +- src/libvirt.c | 6 +-- src/libxl/libxl_driver.c | 2 + src/locking/lock_driver_nop.c | 10 ++-- src/locking/lock_manager.c | 7 ++- src/lxc/lxc_driver.c | 2 + src/openvz/openvz_driver.c | 2 + src/phyp/phyp_driver.c | 2 + src/qemu/qemu_driver.c | 107 +++++++++------------------------------- src/qemu/qemu_process.c | 2 +- src/remote/remote_driver.c | 8 +++- src/secret/secret_driver.c | 7 ++- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 2 + src/uml/uml_driver.c | 24 ++++++--- src/util/command.c | 2 +- src/util/iohelper.c | 18 +++--- src/util/util.c | 14 +++--- src/vbox/vbox_tmpl.c | 18 +++++-- src/vmware/vmware_driver.c | 2 + src/xen/xen_driver.c | 18 ++++++- src/xen/xen_driver.h | 7 +++ src/xen/xen_hypervisor.c | 8 ++- src/xen/xen_inotify.c | 4 +- src/xen/xend_internal.c | 42 +++++++++++++--- src/xen/xend_internal.h | 3 +- src/xen/xm_internal.c | 33 ++++++++++--- src/xen/xm_internal.h | 2 +- src/xen/xs_internal.c | 12 +++- src/xenapi/xenapi_driver.c | 4 +- 43 files changed, 323 insertions(+), 222 deletions(-) -- 1.7.4.4
The previous patches only cleaned up ATTRIBUTE_UNUSED flags cases; auditing the drivers found other places where flags was being used but not validated. In particular, domainGetXMLDesc had issues with clients accepting a different set of flags than the common virDomainDefFormat helper function. * src/conf/domain_conf.c (virDomainDefFormat): Add common flag check. * src/storage/storage_driver.c (storageVolumeCreateXMLFrom): Pass 0 to drivers, since all flags are currently rejected. * src/uml/uml_driver.c (umlDomainAttachDeviceFlags) (umlDomainDetachDeviceFlags): Reject unknown flags. * src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc) (vboxDomainAttachDeviceFlags) (vboxDomainDetachDeviceFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainMemoryPeek): Likewise. (qemuDomainGetXMLDesc): Document common flag handling. * src/libxl/libxl_driver.c (libxlDomainGetXMLDesc): Likewise. * src/lxc/lxc_driver.c (lxcDomainGetXMLDesc): Likewise. * src/openvz/openvz_driver.c (openvzDomainGetXMLDesc): Likewise. * src/phyp/phyp_driver.c (phypDomainGetXMLDesc): Likewise. * src/test/test_driver.c (testDomainGetXMLDesc): Likewise. * src/vmware/vmware_driver.c (vmwareDomainGetXMLDesc): Likewise. * src/xenapi/xenapi_driver.c (xenapiDomainGetXMLDesc): Likewise. --- v3: new patch src/conf/domain_conf.c | 4 ++++ src/libxl/libxl_driver.c | 2 ++ src/lxc/lxc_driver.c | 2 ++ src/openvz/openvz_driver.c | 2 ++ src/phyp/phyp_driver.c | 2 ++ src/qemu/qemu_driver.c | 6 +++--- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 2 ++ src/uml/uml_driver.c | 24 +++++++++++++++--------- src/vbox/vbox_tmpl.c | 18 ++++++++++++++---- src/vmware/vmware_driver.c | 2 ++ src/xenapi/xenapi_driver.c | 4 +--- 12 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a78996f..788981f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9624,6 +9624,10 @@ char *virDomainDefFormat(virDomainDefPtr def, const char *type = NULL; int n, allones = 1; + virCheckFlags(VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_UPDATE_CPU, NULL); + if (!(type = virDomainVirtTypeToString(def->virtType))) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected domain type %d"), def->virtType); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f938e24..cc37d05 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2494,6 +2494,8 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; char *ret = NULL; + /* Flags checked by virDomainDefFormat */ + libxlDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); libxlDriverUnlock(driver); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b6da757..78f0f9d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -967,6 +967,8 @@ static char *lxcDomainGetXMLDesc(virDomainPtr dom, virDomainObjPtr vm; char *ret = NULL; + /* Flags checked by virDomainDefFormat */ + lxcDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); lxcDriverUnlock(driver); diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d50ecf1..cc0c590 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -461,6 +461,8 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { virDomainObjPtr vm; char *ret = NULL; + /* Flags checked by virDomainDefFormat */ + openvzDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); openvzDriverUnlock(driver); diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 62ba192..2489063 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3317,6 +3317,8 @@ phypDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDef def; char *managed_system = phyp_driver->managed_system; + /* Flags checked by virDomainDefFormat */ + memset(&def, 0, sizeof(virDomainDef)); def.virtType = VIR_DOMAIN_VIRT_PHYP; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d4207e..c0acf52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3840,9 +3840,7 @@ static char *qemuDomainGetXMLDesc(virDomainPtr dom, unsigned long balloon; int err; - virCheckFlags(VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_UPDATE_CPU, NULL); + /* Flags checked by virDomainDefFormat */ qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6273,6 +6271,8 @@ qemudDomainMemoryPeek (virDomainPtr dom, int fd = -1, ret = -1; qemuDomainObjPrivatePtr priv; + virCheckFlags(VIR_MEMORY_VIRTUAL | VIR_MEMORY_PHYSICAL, -1); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f35be0..419df42 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1522,7 +1522,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjUnlock(origpool); } - buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags); + buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, 0); storageDriverLock(driver); virStoragePoolObjLock(pool); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f3fb320..064a1cd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2366,6 +2366,8 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr privdom; char *ret = NULL; + /* Flags checked by virDomainDefFormat */ + testDriverLock(privconn); privdom = virDomainFindByName(&privconn->domains, domain->name); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 6eede55..0f4f60e 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1586,9 +1586,7 @@ static char *umlDomainGetXMLDesc(virDomainPtr dom, virDomainObjPtr vm; char *ret = NULL; - virCheckFlags(VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_UPDATE_CPU, NULL); + /* Flags checked by virDomainDefFormat */ umlDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -1855,9 +1853,13 @@ cleanup: } -static int umlDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { +static int +umlDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { umlReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify the persistent configuration of a domain")); @@ -1963,9 +1965,13 @@ cleanup: } -static int umlDomainDetachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { +static int +umlDomainDetachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { umlReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify the persistent configuration of a domain")); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 6cb9bb4..53bac79 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2145,6 +2145,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { nsresult rc; char *tmp; + /* Flags checked by virDomainDefFormat */ + if (VIR_ALLOC(def) < 0) { virReportOOMError(); goto cleanup; @@ -5296,8 +5298,12 @@ static int vboxDomainAttachDevice(virDomainPtr dom, const char *xml) { return vboxDomainAttachDeviceImpl(dom, xml, 0); } -static int vboxDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags) { +static int +vboxDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) +{ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { vboxError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify the persistent configuration of a domain")); @@ -5446,8 +5452,12 @@ cleanup: return ret; } -static int vboxDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, - unsigned int flags) { +static int +vboxDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, + unsigned int flags) +{ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { vboxError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify the persistent configuration of a domain")); diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index c0430fe..36b48e0 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -807,6 +807,8 @@ vmwareDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; char *ret = NULL; + /* Flags checked by virDomainDefFormat */ + vmwareDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); vmwareDriverUnlock(driver); diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 98838e6..97da1d1 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1318,9 +1318,7 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) struct xen_vif_set *vif_set = NULL; char *xml; - virCheckFlags(VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_UPDATE_CPU, NULL); + /* Flags checked by virDomainDefFormat */ if (!xen_vm_get_by_name_label(session, &vms, dom->name)) return NULL; if (vms->size != 1) { -- 1.7.4.4
2011/7/15 Eric Blake <eblake@redhat.com>:
The previous patches only cleaned up ATTRIBUTE_UNUSED flags cases; auditing the drivers found other places where flags was being used but not validated. In particular, domainGetXMLDesc had issues with clients accepting a different set of flags than the common virDomainDefFormat helper function.
* src/conf/domain_conf.c (virDomainDefFormat): Add common flag check. * src/storage/storage_driver.c (storageVolumeCreateXMLFrom): Pass 0 to drivers, since all flags are currently rejected. * src/uml/uml_driver.c (umlDomainAttachDeviceFlags) (umlDomainDetachDeviceFlags): Reject unknown flags. * src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc) (vboxDomainAttachDeviceFlags) (vboxDomainDetachDeviceFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainMemoryPeek): Likewise. (qemuDomainGetXMLDesc): Document common flag handling. * src/libxl/libxl_driver.c (libxlDomainGetXMLDesc): Likewise. * src/lxc/lxc_driver.c (lxcDomainGetXMLDesc): Likewise. * src/openvz/openvz_driver.c (openvzDomainGetXMLDesc): Likewise. * src/phyp/phyp_driver.c (phypDomainGetXMLDesc): Likewise. * src/test/test_driver.c (testDomainGetXMLDesc): Likewise. * src/vmware/vmware_driver.c (vmwareDomainGetXMLDesc): Likewise. * src/xenapi/xenapi_driver.c (xenapiDomainGetXMLDesc): Likewise. ---
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f35be0..419df42 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1522,7 +1522,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjUnlock(origpool); }
- buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags); + buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, 0);
storageDriverLock(driver); virStoragePoolObjLock(pool);
I don't think that this is a good idea. Even if the function doesn't have any flags at the moment this change will give us trouble when someone want's to add a flag to virStorageVolCreateXMLFrom that should have been passed down to the backend's buildVolFrom function. ACK, to a patch without this hunk. -- Matthias Bolte http://photron.blogspot.com
On 07/15/2011 09:25 AM, Matthias Bolte wrote:
2011/7/15 Eric Blake <eblake@redhat.com>:
The previous patches only cleaned up ATTRIBUTE_UNUSED flags cases; auditing the drivers found other places where flags was being used but not validated. In particular, domainGetXMLDesc had issues with clients accepting a different set of flags than the common virDomainDefFormat helper function.
* src/conf/domain_conf.c (virDomainDefFormat): Add common flag check. * src/storage/storage_driver.c (storageVolumeCreateXMLFrom): Pass 0 to drivers, since all flags are currently rejected.
- buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags); + buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, 0);
storageDriverLock(driver); virStoragePoolObjLock(pool);
I don't think that this is a good idea. Even if the function doesn't have any flags at the moment this change will give us trouble when someone want's to add a flag to virStorageVolCreateXMLFrom that should have been passed down to the backend's buildVolFrom function.
Fair point, and hunk withdrawn.
ACK, to a patch without this hunk.
Pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
n 07/14/2011 04:42 PM, Eric Blake wrote:
The previous patches only cleaned up ATTRIBUTE_UNUSED flags cases; auditing the drivers found other places where flags was being used but not validated. In particular, domainGetXMLDesc had issues with clients accepting a different set of flags than the common virDomainDefFormat helper function.
* src/conf/domain_conf.c (virDomainDefFormat): Add common flag check.
Ouch, another regression. virDomainDefFormat is now causing migration failure, because virDomainObjFormat (called by virDomainSaveStatus) is passing an internal-only flag. I'll have a fix for that shortly. :( -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/qemu/qemu_driver.c (QEMU_MIGRATION_FLAGS): New define. Simplify all migration callbacks. --- v3: new patch src/qemu/qemu_driver.c | 101 ++++++++++-------------------------------------- 1 files changed, 21 insertions(+), 80 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0acf52..54292e1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,6 +112,17 @@ #define QEMU_NB_BLKIO_PARAM 1 +/* All supported qemu migration flags. */ +#define QEMU_MIGRATION_FLAGS \ + (VIR_MIGRATE_LIVE | \ + VIR_MIGRATE_PEER2PEER | \ + VIR_MIGRATE_TUNNELLED | \ + VIR_MIGRATE_PERSIST_DEST | \ + VIR_MIGRATE_UNDEFINE_SOURCE | \ + VIR_MIGRATE_PAUSED | \ + VIR_MIGRATE_NON_SHARED_DISK | \ + VIR_MIGRATE_NON_SHARED_INC) + static void processWatchdogEvent(void *data, void *opaque); static int qemudShutdown(void); @@ -6625,14 +6636,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, struct qemud_driver *driver = dconn->privateData; int ret = -1; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); qemuDriverLock(driver); @@ -6686,14 +6690,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, struct qemud_driver *driver = dconn->privateData; int ret = -1; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); *uri_out = NULL; @@ -6751,14 +6748,7 @@ qemudDomainMigratePerform (virDomainPtr dom, int ret = -1; const char *dconnuri = NULL; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); qemuDriverLock(driver); if (virLockManagerPluginUsesState(driver->lockManager)) { @@ -6813,14 +6803,7 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, virDomainObjPtr vm; virDomainPtr dom = NULL; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, NULL); + virCheckFlags(QEMU_MIGRATION_FLAGS, NULL); qemuDriverLock(driver); vm = virDomainFindByName(&driver->domains, dname); @@ -6861,14 +6844,7 @@ qemuDomainMigrateBegin3(virDomainPtr domain, virDomainObjPtr vm; char *xml = NULL; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, NULL); + virCheckFlags(QEMU_MIGRATION_FLAGS, NULL); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -6904,14 +6880,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, struct qemud_driver *driver = dconn->privateData; int ret = -1; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); *uri_out = NULL; @@ -6958,14 +6927,7 @@ qemuDomainMigratePrepareTunnel3(virConnectPtr dconn, struct qemud_driver *driver = dconn->privateData; int ret = -1; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); if (!dom_xml) { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -7012,14 +6974,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -7058,14 +7013,7 @@ qemuDomainMigrateFinish3(virConnectPtr dconn, virDomainObjPtr vm; virDomainPtr dom = NULL; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, NULL); + virCheckFlags(QEMU_MIGRATION_FLAGS, NULL); qemuDriverLock(driver); vm = virDomainFindByName(&driver->domains, dname); @@ -7096,14 +7044,7 @@ qemuDomainMigrateConfirm3(virDomainPtr domain, virDomainObjPtr vm; int ret = -1; - virCheckFlags(VIR_MIGRATE_LIVE | - VIR_MIGRATE_PEER2PEER | - VIR_MIGRATE_TUNNELLED | - VIR_MIGRATE_PERSIST_DEST | - VIR_MIGRATE_UNDEFINE_SOURCE | - VIR_MIGRATE_PAUSED | - VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); -- 1.7.4.4
2011/7/15 Eric Blake <eblake@redhat.com>:
* src/qemu/qemu_driver.c (QEMU_MIGRATION_FLAGS): New define. Simplify all migration callbacks. ---
v3: new patch
src/qemu/qemu_driver.c | 101 ++++++++++-------------------------------------- 1 files changed, 21 insertions(+), 80 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
On 07/15/2011 09:27 AM, Matthias Bolte wrote:
2011/7/15 Eric Blake <eblake@redhat.com>:
* src/qemu/qemu_driver.c (QEMU_MIGRATION_FLAGS): New define. Simplify all migration callbacks. ---
v3: new patch
src/qemu/qemu_driver.c | 101 ++++++++++-------------------------------------- 1 files changed, 21 insertions(+), 80 deletions(-)
ACK.
Ouch. Stefan pointed out on IRC that I added a virCheckFlags(0,01) in qemuMigrationConfirm() which is indeed a regression that breaks qemu migration. I'll have to fix that before pushing this. :( -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/15/2011 12:26 PM, Eric Blake wrote:
On 07/15/2011 09:27 AM, Matthias Bolte wrote:
2011/7/15 Eric Blake <eblake@redhat.com>:
* src/qemu/qemu_driver.c (QEMU_MIGRATION_FLAGS): New define. Simplify all migration callbacks. ---
v3: new patch
src/qemu/qemu_driver.c | 101 ++++++++++-------------------------------------- 1 files changed, 21 insertions(+), 80 deletions(-)
ACK.
Ouch. Stefan pointed out on IRC that I added a virCheckFlags(0,01) in qemuMigrationConfirm() which is indeed a regression that breaks qemu migration. I'll have to fix that before pushing this. :(
Found it, so I'm squashing this in, as well as updating the commit message: flags: fix qemu migration regression Commit f548480b broke migration v3 on qemu, because the driver passed flags on through to qemu_migration even though qemu_migration wasn't using those flags. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 3e0479c..8d146aa 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -112,17 +112,6 @@ #define QEMU_NB_BLKIO_PARAM 1 -/* All supported qemu migration flags. */ -#define QEMU_MIGRATION_FLAGS \ - (VIR_MIGRATE_LIVE | \ - VIR_MIGRATE_PEER2PEER | \ - VIR_MIGRATE_TUNNELLED | \ - VIR_MIGRATE_PERSIST_DEST | \ - VIR_MIGRATE_UNDEFINE_SOURCE | \ - VIR_MIGRATE_PAUSED | \ - VIR_MIGRATE_NON_SHARED_DISK | \ - VIR_MIGRATE_NON_SHARED_INC) - static void processWatchdogEvent(void *data, void *opaque); static int qemudShutdown(void); diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index b8f563b..dfa80e3 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -2575,7 +2575,7 @@ int qemuMigrationConfirm(struct qemud_driver *driver, driver, conn, vm, NULLSTR(cookiein), cookieinlen, flags, retcode); - virCheckFlags(0, -1); + virCheckFlags(QEMU_MIGRATION_FLAGS, -1); if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, 0))) return -1; diff --git i/src/qemu/qemu_migration.h w/src/qemu/qemu_migration.h index d3a3743..df3f06f 100644 --- i/src/qemu/qemu_migration.h +++ w/src/qemu/qemu_migration.h @@ -24,6 +24,16 @@ # include "qemu_conf.h" +/* All supported qemu migration flags. */ +#define QEMU_MIGRATION_FLAGS \ + (VIR_MIGRATE_LIVE | \ + VIR_MIGRATE_PEER2PEER | \ + VIR_MIGRATE_TUNNELLED | \ + VIR_MIGRATE_PERSIST_DEST | \ + VIR_MIGRATE_UNDEFINE_SOURCE | \ + VIR_MIGRATE_PAUSED | \ + VIR_MIGRATE_NON_SHARED_DISK | \ + VIR_MIGRATE_NON_SHARED_INC) bool qemuMigrationIsAllowed(virDomainDefPtr def) ATTRIBUTE_NONNULL(1);
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Silently ignored flags get in the way of new features that use those flags. Regarding ESX migration flags - right now, ESX silently enforces VIR_MIGRATE_PERSIST_DEST, VIR_MIGRATE_UNDEFINE_SOURCE, and VIR_MIGRATE_LIVE, even if those flags were not supplied; it ignored other flags. This patch does not change the implied bits (it permits but does not require them), but enforces only the supported bits. If further cleanup is needed to be more particular about migration flags, that should be a separate patch. * src/esx/esx_device_monitor.c (esxDeviceOpen): Reject unknown flags. * src/esx/esx_driver.c (esxOpen, esxDomainReboot) (esxDomainXMLFromNative, esxDomainXMLToNative) (esxDomainMigratePrepare, esxDomainMigratePerform) (esxDomainMigrateFinish): Likewise. * src/esx/esx_interface_driver.c (esxInterfaceOpen): Likewise. * src/esx/esx_network_driver.c (esxNetworkOpen): Likewise. * src/esx/esx_nwfilter_driver.c (esxNWFilterOpen): Likewise. * src/esx/esx_secret_driver.c (esxSecretOpen): Likewise. * src/esx/esx_storage_driver.c (esxStorageOpen): Likewise. --- v3: address concerns in v2 about migration flags src/esx/esx_device_monitor.c | 4 +++- src/esx/esx_driver.c | 35 ++++++++++++++++++++++++++++------- src/esx/esx_interface_driver.c | 4 +++- src/esx/esx_network_driver.c | 4 +++- src/esx/esx_nwfilter_driver.c | 4 +++- src/esx/esx_secret_driver.c | 4 +++- src/esx/esx_storage_driver.c | 4 +++- 7 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/esx/esx_device_monitor.c b/src/esx/esx_device_monitor.c index 51b2413..2eba2b0 100644 --- a/src/esx/esx_device_monitor.c +++ b/src/esx/esx_device_monitor.c @@ -42,8 +42,10 @@ static virDrvOpenStatus esxDeviceOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (conn->driver->no != VIR_DRV_ESX) { return VIR_DRV_OPEN_DECLINED; } diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4643a32..80d687b 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -935,13 +935,15 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth, */ static virDrvOpenStatus esxOpen(virConnectPtr conn, virConnectAuthPtr auth, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { virDrvOpenStatus result = VIR_DRV_OPEN_ERROR; esxPrivate *priv = NULL; char *potentialVCenterIpAddress = NULL; char vCenterIpAddress[NI_MAXHOST] = ""; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + /* Decline if the URI is NULL or the scheme is not one of {vpx|esx|gsx} */ if (conn->uri == NULL || conn->uri->scheme == NULL || (STRCASENEQ(conn->uri->scheme, "vpx") && @@ -1890,7 +1892,7 @@ esxDomainShutdown(virDomainPtr domain) static int -esxDomainReboot(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED) +esxDomainReboot(virDomainPtr domain, unsigned int flags) { int result = -1; esxPrivate *priv = domain->conn->privateData; @@ -1898,6 +1900,8 @@ esxDomainReboot(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED) esxVI_String *propertyNameList = NULL; esxVI_VirtualMachinePowerState powerState; + virCheckFlags(0, -1); + if (esxVI_EnsureSession(priv->primary) < 0) { return -1; } @@ -2701,6 +2705,8 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainDefPtr def = NULL; char *xml = NULL; + /* Flags checked by virDomainDefFormat */ + memset(&data, 0, sizeof (data)); if (esxVI_EnsureSession(priv->primary) < 0) { @@ -2798,7 +2804,7 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) static char * esxDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, const char *nativeConfig, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = conn->privateData; virVMXContext ctx; @@ -2806,6 +2812,8 @@ esxDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, virDomainDefPtr def = NULL; char *xml = NULL; + virCheckFlags(0, NULL); + memset(&data, 0, sizeof (data)); if (STRNEQ(nativeFormat, "vmware-vmx")) { @@ -2838,7 +2846,7 @@ esxDomainXMLFromNative(virConnectPtr conn, const char *nativeFormat, static char * esxDomainXMLToNative(virConnectPtr conn, const char *nativeFormat, const char *domainXml, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = conn->privateData; int virtualHW_version; @@ -2847,6 +2855,8 @@ esxDomainXMLToNative(virConnectPtr conn, const char *nativeFormat, virDomainDefPtr def = NULL; char *vmx = NULL; + virCheckFlags(0, NULL); + memset(&data, 0, sizeof (data)); if (STRNEQ(nativeFormat, "vmware-vmx")) { @@ -3831,6 +3841,11 @@ esxDomainSetSchedulerParameters(virDomainPtr domain, return esxDomainSetSchedulerParametersFlags(domain, params, nparams, 0); } +/* The subset of migration flags we are able to support. */ +#define ESX_MIGRATION_FLAGS \ + (VIR_MIGRATE_PERSIST_DEST | \ + VIR_MIGRATE_UNDEFINE_SOURCE | \ + VIR_MIGRATE_LIVE) static int esxDomainMigratePrepare(virConnectPtr dconn, @@ -3838,12 +3853,14 @@ esxDomainMigratePrepare(virConnectPtr dconn, int *cookielen ATTRIBUTE_UNUSED, const char *uri_in ATTRIBUTE_UNUSED, char **uri_out, - unsigned long flags ATTRIBUTE_UNUSED, + unsigned long flags, const char *dname ATTRIBUTE_UNUSED, unsigned long resource ATTRIBUTE_UNUSED) { esxPrivate *priv = dconn->privateData; + virCheckFlags(ESX_MIGRATION_FLAGS, -1); + if (uri_in == NULL) { if (virAsprintf(uri_out, "vpxmigr://%s/%s/%s", priv->vCenter->ipAddress, @@ -3864,7 +3881,7 @@ esxDomainMigratePerform(virDomainPtr domain, const char *cookie ATTRIBUTE_UNUSED, int cookielen ATTRIBUTE_UNUSED, const char *uri, - unsigned long flags ATTRIBUTE_UNUSED, + unsigned long flags, const char *dname, unsigned long bandwidth ATTRIBUTE_UNUSED) { @@ -3882,6 +3899,8 @@ esxDomainMigratePerform(virDomainPtr domain, esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; + virCheckFlags(ESX_MIGRATION_FLAGS, -1); + if (priv->vCenter == NULL) { ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", _("Migration not possible without a vCenter")); @@ -4010,8 +4029,10 @@ esxDomainMigrateFinish(virConnectPtr dconn, const char *dname, const char *cookie ATTRIBUTE_UNUSED, int cookielen ATTRIBUTE_UNUSED, const char *uri ATTRIBUTE_UNUSED, - unsigned long flags ATTRIBUTE_UNUSED) + unsigned long flags) { + virCheckFlags(ESX_MIGRATION_FLAGS, NULL); + return esxDomainLookupByName(dconn, dname); } diff --git a/src/esx/esx_interface_driver.c b/src/esx/esx_interface_driver.c index 3d23d0f..5713137 100644 --- a/src/esx/esx_interface_driver.c +++ b/src/esx/esx_interface_driver.c @@ -42,8 +42,10 @@ static virDrvOpenStatus esxInterfaceOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (conn->driver->no != VIR_DRV_ESX) { return VIR_DRV_OPEN_DECLINED; } diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c index f8a3ede..224c764 100644 --- a/src/esx/esx_network_driver.c +++ b/src/esx/esx_network_driver.c @@ -42,8 +42,10 @@ static virDrvOpenStatus esxNetworkOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (conn->driver->no != VIR_DRV_ESX) { return VIR_DRV_OPEN_DECLINED; } diff --git a/src/esx/esx_nwfilter_driver.c b/src/esx/esx_nwfilter_driver.c index d7fa15a..6056d6d 100644 --- a/src/esx/esx_nwfilter_driver.c +++ b/src/esx/esx_nwfilter_driver.c @@ -42,8 +42,10 @@ static virDrvOpenStatus esxNWFilterOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (conn->driver->no != VIR_DRV_ESX) { return VIR_DRV_OPEN_DECLINED; } diff --git a/src/esx/esx_secret_driver.c b/src/esx/esx_secret_driver.c index 820f4b1..c37b62d 100644 --- a/src/esx/esx_secret_driver.c +++ b/src/esx/esx_secret_driver.c @@ -40,8 +40,10 @@ static virDrvOpenStatus esxSecretOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (conn->driver->no != VIR_DRV_ESX) { return VIR_DRV_OPEN_DECLINED; } diff --git a/src/esx/esx_storage_driver.c b/src/esx/esx_storage_driver.c index 528159a..a56536d 100644 --- a/src/esx/esx_storage_driver.c +++ b/src/esx/esx_storage_driver.c @@ -104,8 +104,10 @@ esxStoragePoolLookupType(esxVI_Context *ctx, const char *poolName, static virDrvOpenStatus esxStorageOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (conn->driver->no != VIR_DRV_ESX) { return VIR_DRV_OPEN_DECLINED; } -- 1.7.4.4
2011/7/15 Eric Blake <eblake@redhat.com>:
Silently ignored flags get in the way of new features that use those flags.
Regarding ESX migration flags - right now, ESX silently enforces VIR_MIGRATE_PERSIST_DEST, VIR_MIGRATE_UNDEFINE_SOURCE, and VIR_MIGRATE_LIVE, even if those flags were not supplied; it ignored other flags. This patch does not change the implied bits (it permits but does not require them), but enforces only the supported bits. If further cleanup is needed to be more particular about migration flags, that should be a separate patch.
* src/esx/esx_device_monitor.c (esxDeviceOpen): Reject unknown flags. * src/esx/esx_driver.c (esxOpen, esxDomainReboot) (esxDomainXMLFromNative, esxDomainXMLToNative) (esxDomainMigratePrepare, esxDomainMigratePerform) (esxDomainMigrateFinish): Likewise. * src/esx/esx_interface_driver.c (esxInterfaceOpen): Likewise. * src/esx/esx_network_driver.c (esxNetworkOpen): Likewise. * src/esx/esx_nwfilter_driver.c (esxNWFilterOpen): Likewise. * src/esx/esx_secret_driver.c (esxSecretOpen): Likewise. * src/esx/esx_storage_driver.c (esxStorageOpen): Likewise. ---
v3: address concerns in v2 about migration flags
src/esx/esx_device_monitor.c | 4 +++- src/esx/esx_driver.c | 35 ++++++++++++++++++++++++++++------- src/esx/esx_interface_driver.c | 4 +++- src/esx/esx_network_driver.c | 4 +++- src/esx/esx_nwfilter_driver.c | 4 +++- src/esx/esx_secret_driver.c | 4 +++- src/esx/esx_storage_driver.c | 4 +++- 7 files changed, 46 insertions(+), 13 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
On 07/15/2011 09:30 AM, Matthias Bolte wrote:
2011/7/15 Eric Blake<eblake@redhat.com>:
Silently ignored flags get in the way of new features that use those flags.
Regarding ESX migration flags - right now, ESX silently enforces VIR_MIGRATE_PERSIST_DEST, VIR_MIGRATE_UNDEFINE_SOURCE, and VIR_MIGRATE_LIVE, even if those flags were not supplied; it ignored other flags. This patch does not change the implied bits (it permits but does not require them), but enforces only the supported bits. If further cleanup is needed to be more particular about migration flags, that should be a separate patch.
It turns out that libvirt.c also handles (or at least claims to handle) VIR_MIGRATE_PAUSED on behalf of ESX, so I'm squashing this in: diff --git i/src/esx/esx_driver.c w/src/esx/esx_driver.c index 80d687b..9b0a541 100644 --- i/src/esx/esx_driver.c +++ w/src/esx/esx_driver.c @@ -3845,7 +3845,8 @@ esxDomainSetSchedulerParameters(virDomainPtr domain, #define ESX_MIGRATION_FLAGS \ (VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE | \ - VIR_MIGRATE_LIVE) + VIR_MIGRATE_LIVE | \ + VIR_MIGRATE_PAUSED) static int esxDomainMigratePrepare(virConnectPtr dconn, libvirt.c also handles VIR_MIGRATE_PEER2PEER and VIR_MIGRATE_TUNNELLED, but only if the driver advertises those features, which ESX does not. So by the time the ESX callbacks are reached, those flag bits can never be set.
ACK.
I've pushed this now. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Also fix a logic bug in xenXMDomain{Attach,Detach}DeviceFlags, where (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT) is always false. * src/xen/xen_driver.c (xenUnifiedDomainXMLFromNative) (xenUnifiedDomainXMLToNative, xenUnifiedDomainBlockPeek): Reject unknown flags. * src/xen/xen_hypervisor.c (xenHypervisorOpen) (xenHypervisorGetDomainState): Likewise. * src/xen/xen_inotify.c (xenInotifyOpen): Likewise. * src/xen/xs_internal.c (xenStoreOpen, xenStoreDomainGetState) (xenStoreDomainReboot): Likewise. * src/xen/xend_internal.c (xenDaemonOpen, xenDaemonDomainReboot) (xenDaemonDomainCoreDump, xenDaemonDomainGetState) (xenDaemonDomainMigratePrepare, xenDaemonDomainSetVcpusFlags, xenDaemonDomainGetVcpusFlags, xenDaemonAttachDeviceFlags, xenDaemonDetachDeviceFlags): Likewise. (xenDaemonDomainGetXMLDesc): Prefer unsigned flags. * src/xen/xend_internal.h (xenDaemonDomainGetXMLDesc): Likewise. * src/xen/xm_internal.h (xenXMDomainGetXMLDesc): Likewise. * src/xen/xm_internal.c (xenXMDomainGetXMLDesc): Likewise. (xenXMOpen, xenXMDomainGetState, xenXMDomainSetVcpusFlags) (xenXMDomainGetVcpusFlags): Reject unknown flags. (xenXMDomainAttachDeviceFlags, xenXMDomainDetachDeviceFlags): Likewise, and avoid always-false conditional. * src/xen/xen_driver.h (XEN_MIGRATION_FLAGS): New define. --- v3: address concerns about migration and coredump flags src/xen/xen_driver.c | 18 +++++++++++++++--- src/xen/xen_driver.h | 7 +++++++ src/xen/xen_hypervisor.c | 8 ++++++-- src/xen/xen_inotify.c | 4 +++- src/xen/xend_internal.c | 42 ++++++++++++++++++++++++++++++++++-------- src/xen/xend_internal.h | 3 ++- src/xen/xm_internal.c | 33 ++++++++++++++++++++++++++------- src/xen/xm_internal.h | 2 +- src/xen/xs_internal.c | 12 +++++++++--- 9 files changed, 103 insertions(+), 26 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 1d75da3..dd1ba6c 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1261,7 +1261,7 @@ static char * xenUnifiedDomainXMLFromNative(virConnectPtr conn, const char *format, const char *config, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { virDomainDefPtr def = NULL; char *ret = NULL; @@ -1271,6 +1271,8 @@ xenUnifiedDomainXMLFromNative(virConnectPtr conn, int vncport; GET_PRIVATE(conn); + virCheckFlags(0, NULL); + if (STRNEQ(format, XEN_CONFIG_FORMAT_XM) && STRNEQ(format, XEN_CONFIG_FORMAT_SEXPR)) { xenUnifiedError(VIR_ERR_INVALID_ARG, @@ -1311,13 +1313,15 @@ static char * xenUnifiedDomainXMLToNative(virConnectPtr conn, const char *format, const char *xmlData, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { virDomainDefPtr def = NULL; char *ret = NULL; virConfPtr conf = NULL; GET_PRIVATE(conn); + virCheckFlags(0, NULL); + if (STRNEQ(format, XEN_CONFIG_FORMAT_XM) && STRNEQ(format, XEN_CONFIG_FORMAT_SEXPR)) { xenUnifiedError(VIR_ERR_INVALID_ARG, @@ -1368,6 +1372,8 @@ xenUnifiedDomainMigratePrepare (virConnectPtr dconn, { GET_PRIVATE(dconn); + virCheckFlags(XEN_MIGRATION_FLAGS, -1); + if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) return xenDaemonDomainMigratePrepare (dconn, cookie, cookielen, uri_in, uri_out, @@ -1388,6 +1394,8 @@ xenUnifiedDomainMigratePerform (virDomainPtr dom, { GET_PRIVATE(dom->conn); + virCheckFlags(XEN_MIGRATION_FLAGS, -1); + if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) return xenDaemonDomainMigratePerform (dom, cookie, cookielen, uri, flags, dname, resource); @@ -1408,6 +1416,8 @@ xenUnifiedDomainMigrateFinish (virConnectPtr dconn, char *domain_xml = NULL; virDomainPtr dom_new = NULL; + virCheckFlags(XEN_MIGRATION_FLAGS, NULL); + dom = xenUnifiedDomainLookupByName (dconn, dname); if (! dom) { return NULL; @@ -1765,11 +1775,13 @@ xenUnifiedDomainInterfaceStats (virDomainPtr dom, const char *path, static int xenUnifiedDomainBlockPeek (virDomainPtr dom, const char *path, unsigned long long offset, size_t size, - void *buffer, unsigned int flags ATTRIBUTE_UNUSED) + void *buffer, unsigned int flags) { int r; GET_PRIVATE (dom->conn); + virCheckFlags(0, -1); + if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) { r = xenDaemonDomainBlockPeek (dom, path, offset, size, buffer); if (r != -2) return r; diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index 6d45f7d..a6fe475 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -63,6 +63,13 @@ extern int xenRegister (void); # define XEN_SCHED_SEDF_NPARAM 6 # define XEN_SCHED_CRED_NPARAM 2 +/* The set of migration flags explicitly supported by xen. */ +# define XEN_MIGRATION_FLAGS \ + (VIR_MIGRATE_LIVE | \ + VIR_MIGRATE_UNDEFINE_SOURCE | \ + VIR_MIGRATE_PAUSED | \ + VIR_MIGRATE_PERSIST_DEST) + /* _xenUnifiedDriver: * * Entry points into the underlying Xen drivers. This structure diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index a92b743..543dfb1 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -2201,11 +2201,13 @@ xenHypervisorInit(void) virDrvOpenStatus xenHypervisorOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { int ret; xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (initialized == 0) if (xenHypervisorInit() == -1) return -1; @@ -3272,11 +3274,13 @@ int xenHypervisorGetDomainState(virDomainPtr domain, int *state, int *reason, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { xenUnifiedPrivatePtr priv = domain->conn->privateData; virDomainInfo info; + virCheckFlags(0, -1); + if (domain->conn == NULL) return -1; diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index 2d7207c..241dbc7 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -383,13 +383,15 @@ cleanup: virDrvOpenStatus xenInotifyOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { DIR *dh; struct dirent *ent; char *path; xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (priv->configDir) { priv->useXenConfigCache = 1; } else { diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d0679b7..dec8484 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1323,11 +1323,13 @@ error: virDrvOpenStatus xenDaemonOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { char *port = NULL; int ret = VIR_DRV_OPEN_ERROR; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + /* Switch on the scheme, which we expect to be NULL (file), * "http" or "xen". */ @@ -1488,8 +1490,10 @@ xenDaemonDomainShutdown(virDomainPtr domain) * Returns 0 in case of success, -1 (with errno) in case of error. */ int -xenDaemonDomainReboot(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED) +xenDaemonDomainReboot(virDomainPtr domain, unsigned int flags) { + virCheckFlags(0, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return(-1); @@ -1629,8 +1633,10 @@ xenDaemonDomainSave(virDomainPtr domain, const char *filename) */ static int xenDaemonDomainCoreDump(virDomainPtr domain, const char *filename, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL) || (filename == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -1837,12 +1843,15 @@ cleanup: * the caller must free() the returned value. */ char * -xenDaemonDomainGetXMLDesc(virDomainPtr domain, int flags, const char *cpus) +xenDaemonDomainGetXMLDesc(virDomainPtr domain, unsigned int flags, + const char *cpus) { xenUnifiedPrivatePtr priv; virDomainDefPtr def; char *xml; + /* Flags checked by virDomainDefFormat */ + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return(NULL); @@ -1921,11 +1930,13 @@ int xenDaemonDomainGetState(virDomainPtr domain, int *state, int *reason, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { xenUnifiedPrivatePtr priv = domain->conn->privateData; struct sexpr *root; + virCheckFlags(0, -1); + if (domain->id < 0 && priv->xendConfigVersion < 3) return -1; @@ -2213,6 +2224,10 @@ xenDaemonDomainSetVcpusFlags(virDomainPtr domain, unsigned int vcpus, xenUnifiedPrivatePtr priv; int max; + virCheckFlags(VIR_DOMAIN_VCPU_LIVE | + VIR_DOMAIN_VCPU_CONFIG | + VIR_DOMAIN_VCPU_MAXIMUM, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL) || (vcpus < 1)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -2364,6 +2379,10 @@ xenDaemonDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) int ret; xenUnifiedPrivatePtr priv; + virCheckFlags(VIR_DOMAIN_VCPU_LIVE | + VIR_DOMAIN_VCPU_CONFIG | + VIR_DOMAIN_VCPU_MAXIMUM, -1); + if (domain == NULL || domain->conn == NULL || domain->name == NULL) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; @@ -2660,6 +2679,8 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, char class[8], ref[80]; char *target = NULL; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; @@ -2825,8 +2846,7 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml, virBuffer buf = VIR_BUFFER_INITIALIZER; char class[8], ref[80]; - virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_CURRENT | - VIR_DOMAIN_DEVICE_MODIFY_LIVE | + virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE | VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1); if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { @@ -2940,6 +2960,8 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, char *xendev = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); @@ -3149,10 +3171,12 @@ xenDaemonDomainMigratePrepare (virConnectPtr dconn, int *cookielen ATTRIBUTE_UNUSED, const char *uri_in, char **uri_out, - unsigned long flags ATTRIBUTE_UNUSED, + unsigned long flags, const char *dname ATTRIBUTE_UNUSED, unsigned long resource ATTRIBUTE_UNUSED) { + virCheckFlags(XEN_MIGRATION_FLAGS, -1); + /* If uri_in is NULL, get the current hostname as a best guess * of how the source host should connect to us. Note that caller * deallocates this string. @@ -3187,6 +3211,8 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, int undefined_source = 0; + virCheckFlags(XEN_MIGRATION_FLAGS, -1); + /* Xen doesn't support renaming domains during migration. */ if (dname) { virXendError(VIR_ERR_NO_SUPPORT, diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h index 5d324da..c4ed9ba 100644 --- a/src/xen/xend_internal.h +++ b/src/xen/xend_internal.h @@ -115,7 +115,8 @@ int xenDaemonDomainGetState(virDomainPtr domain, int *state, int *reason, unsigned int flags); -char *xenDaemonDomainGetXMLDesc(virDomainPtr domain, int flags, const char *cpus); +char *xenDaemonDomainGetXMLDesc(virDomainPtr domain, unsigned int flags, + const char *cpus); unsigned long xenDaemonDomainGetMaxMemory(virDomainPtr domain); char **xenDaemonListDomainsOld(virConnectPtr xend); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 68181d2..6ec295e 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -442,10 +442,12 @@ int xenXMConfigCacheRefresh (virConnectPtr conn) { virDrvOpenStatus xenXMOpen (virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { xenUnifiedPrivatePtr priv = conn->privateData; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + priv->configDir = XM_CONFIG_DIR; priv->configCache = virHashCreate(50, xenXMConfigFree); @@ -485,8 +487,10 @@ int xenXMDomainGetState(virDomainPtr domain, int *state, int *reason, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + virCheckFlags(0, -1); + if (domain->id != -1) return -1; @@ -543,12 +547,15 @@ error: * Turn a config record into a lump of XML describing the * domain, suitable for later feeding for virDomainCreateXML */ -char *xenXMDomainGetXMLDesc(virDomainPtr domain, int flags) { +char *xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) +{ xenUnifiedPrivatePtr priv; const char *filename; xenXMConfCachePtr entry; char *ret = NULL; + /* Flags checked by virDomainDefFormat */ + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { xenXMError(VIR_ERR_INVALID_ARG, __FUNCTION__); return(NULL); @@ -714,6 +721,10 @@ xenXMDomainSetVcpusFlags(virDomainPtr domain, unsigned int vcpus, int ret = -1; int max; + virCheckFlags(VIR_DOMAIN_VCPU_LIVE | + VIR_DOMAIN_VCPU_CONFIG | + VIR_DOMAIN_VCPU_MAXIMUM, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { xenXMError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; @@ -794,6 +805,10 @@ xenXMDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) xenXMConfCachePtr entry; int ret = -2; + virCheckFlags(VIR_DOMAIN_VCPU_LIVE | + VIR_DOMAIN_VCPU_CONFIG | + VIR_DOMAIN_VCPU_MAXIMUM, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { xenXMError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; @@ -1378,7 +1393,8 @@ cleanup: */ static int xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, - unsigned int flags) { + unsigned int flags) +{ const char *filename = NULL; xenXMConfCachePtr entry = NULL; int ret = -1; @@ -1386,6 +1402,8 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, virDomainDefPtr def; xenUnifiedPrivatePtr priv; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if ((!domain) || (!domain->conn) || (!domain->name) || (!xml)) { xenXMError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; @@ -1395,7 +1413,7 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, return -1; if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) || - (domain->id != -1 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT))) { + (domain->id != -1 && flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)) { xenXMError(VIR_ERR_OPERATION_INVALID, "%s", _("Xm driver only supports modifying persistent config")); return -1; @@ -1481,17 +1499,18 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, int i; xenUnifiedPrivatePtr priv; + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if ((!domain) || (!domain->conn) || (!domain->name) || (!xml)) { xenXMError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; } - if (domain->conn->flags & VIR_CONNECT_RO) return -1; if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) || - (domain->id != -1 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT))) { + (domain->id != -1 && flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT)) { xenXMError(VIR_ERR_OPERATION_INVALID, "%s", _("Xm driver only supports modifying persistent config")); return -1; diff --git a/src/xen/xm_internal.h b/src/xen/xm_internal.h index cf315cd..89af23e 100644 --- a/src/xen/xm_internal.h +++ b/src/xen/xm_internal.h @@ -45,7 +45,7 @@ int xenXMDomainGetState(virDomainPtr domain, int *state, int *reason, unsigned int flags); -char *xenXMDomainGetXMLDesc(virDomainPtr domain, int flags); +char *xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags); int xenXMDomainSetMemory(virDomainPtr domain, unsigned long memory); int xenXMDomainSetMaxMemory(virDomainPtr domain, unsigned long memory); unsigned long xenXMDomainGetMaxMemory(virDomainPtr domain); diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 48e450a..f62d716 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -267,10 +267,12 @@ virDomainGetVMInfo(virDomainPtr domain, const char *vm, const char *name) virDrvOpenStatus xenStoreOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) conn->privateData; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (flags & VIR_CONNECT_RO) priv->xshandle = xs_daemon_open_readonly(); else @@ -461,10 +463,12 @@ int xenStoreDomainGetState(virDomainPtr domain, int *state, int *reason, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { char *running; + virCheckFlags(0, -1); + if (domain->id == -1) return -1; @@ -778,11 +782,13 @@ xenStoreDomainShutdown(virDomainPtr domain) * Returns 0 in case of success, -1 in case of error. */ int -xenStoreDomainReboot(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED) +xenStoreDomainReboot(virDomainPtr domain, unsigned int flags) { int ret; xenUnifiedPrivatePtr priv; + virCheckFlags(0, -1); + if ((domain == NULL) || (domain->conn == NULL)) { virXenStoreError(VIR_ERR_INVALID_ARG, __FUNCTION__); return(-1); -- 1.7.4.4
2011/7/15 Eric Blake <eblake@redhat.com>:
Also fix a logic bug in xenXMDomain{Attach,Detach}DeviceFlags, where (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT) is always false.
* src/xen/xen_driver.c (xenUnifiedDomainXMLFromNative) (xenUnifiedDomainXMLToNative, xenUnifiedDomainBlockPeek): Reject unknown flags. * src/xen/xen_hypervisor.c (xenHypervisorOpen) (xenHypervisorGetDomainState): Likewise. * src/xen/xen_inotify.c (xenInotifyOpen): Likewise. * src/xen/xs_internal.c (xenStoreOpen, xenStoreDomainGetState) (xenStoreDomainReboot): Likewise. * src/xen/xend_internal.c (xenDaemonOpen, xenDaemonDomainReboot) (xenDaemonDomainCoreDump, xenDaemonDomainGetState) (xenDaemonDomainMigratePrepare, xenDaemonDomainSetVcpusFlags, xenDaemonDomainGetVcpusFlags, xenDaemonAttachDeviceFlags, xenDaemonDetachDeviceFlags): Likewise. (xenDaemonDomainGetXMLDesc): Prefer unsigned flags. * src/xen/xend_internal.h (xenDaemonDomainGetXMLDesc): Likewise. * src/xen/xm_internal.h (xenXMDomainGetXMLDesc): Likewise. * src/xen/xm_internal.c (xenXMDomainGetXMLDesc): Likewise. (xenXMOpen, xenXMDomainGetState, xenXMDomainSetVcpusFlags) (xenXMDomainGetVcpusFlags): Reject unknown flags. (xenXMDomainAttachDeviceFlags, xenXMDomainDetachDeviceFlags): Likewise, and avoid always-false conditional. * src/xen/xen_driver.h (XEN_MIGRATION_FLAGS): New define. ---
v3: address concerns about migration and coredump flags
src/xen/xen_driver.c | 18 +++++++++++++++--- src/xen/xen_driver.h | 7 +++++++ src/xen/xen_hypervisor.c | 8 ++++++-- src/xen/xen_inotify.c | 4 +++- src/xen/xend_internal.c | 42 ++++++++++++++++++++++++++++++++++-------- src/xen/xend_internal.h | 3 ++- src/xen/xm_internal.c | 33 ++++++++++++++++++++++++++------- src/xen/xm_internal.h | 2 +- src/xen/xs_internal.c | 12 +++++++++--- 9 files changed, 103 insertions(+), 26 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
On 07/15/2011 09:36 AM, Matthias Bolte wrote:
2011/7/15 Eric Blake<eblake@redhat.com>:
Also fix a logic bug in xenXMDomain{Attach,Detach}DeviceFlags, where (flags& VIR_DOMAIN_DEVICE_MODIFY_CURRENT) is always false.
ACK.
Pushed. I double-checked that there aren't any additional migration flags handled by libvirt.c that could leak into xen driver callbacks. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
There were two API in driver.c that were silently masking flags bits prior to calling out to the drivers. This is not forward-compatible - if we ever have that many flags in the future, then talking to an old server that masks out the flags would be indistinguishable from talking to a new server that can honor the flag. In general, libvirt.c should forward _all_ flags on to drivers, and only the drivers should reject unknown flags. In the case of virDrvSecretGetValue, the solution is to separate the internal driver callback function to have two parameters instead of one, with only one parameter affected by the public API. In the case of virDomainGetXMLDesc, it turns out that no one was ever mixing VIR_DOMAIN_XML_INTERNAL_STATUS with the dumpxml path in the first place; that internal flag was only used in saving and restoring state files, which happened to be in functions internal to a single file, so there is no mixing of the internal flag with a public flags argument. * src/driver.h (VIR_DOMAIN_XML_FLAGS_MASK) (VIR_SECRET_GET_VALUE_FLAGS_MASK): Delete. (virDrvSecretGetValue): Separate out internal flags. * src/driver.c (verify): Drop unused check. * src/conf/domain_conf.h (virDomainObjParseFile): Delete declaration. (virDomainXMLInternalFlags): Move... * src/conf/domain_conf.c: ...here. (virDomainDefFormat): Tighten validations on internal flags. (virDomainObjParseFile): Make static. * src/libvirt.c (virDomainGetXMLDesc, virSecretGetValue): Update clients. * src/secret/secret_driver.c (secretGetValue): Likewise. * src/remote/remote_driver.c (remoteSecretGetValue): Likewise. * src/qemu/qemu_process.c (qemuProcessGetVolumeQcowPassphrase): Likewise. --- v3: new patch src/conf/domain_conf.c | 27 +++++++++++++++++++-------- src/conf/domain_conf.h | 10 ---------- src/driver.c | 4 ---- src/driver.h | 13 ++++--------- src/libvirt.c | 6 +----- src/qemu/qemu_process.c | 2 +- src/remote/remote_driver.c | 8 +++++++- src/secret/secret_driver.c | 7 +++++-- 8 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 788981f..1667688 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -48,7 +48,6 @@ #include "storage_file.h" #include "files.h" #include "bitmap.h" -#include "verify.h" #include "count-one-bits.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -57,6 +56,12 @@ * verify that it doesn't overflow an unsigned int when shifting */ verify(VIR_DOMAIN_VIRT_LAST <= 32); +/* Private flag used internally by virDomainSaveStatus and + * virDomainObjParseFile. */ +typedef enum { + VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ +} virDomainXMLInternalFlags; + VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "custom-argv", "custom-monitor", @@ -6995,10 +7000,11 @@ cleanup: } -virDomainObjPtr virDomainObjParseFile(virCapsPtr caps, - const char *filename, - unsigned int expectedVirtTypes, - unsigned int flags) +static virDomainObjPtr +virDomainObjParseFile(virCapsPtr caps, + const char *filename, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlDocPtr xml; virDomainObjPtr obj = NULL; @@ -9615,6 +9621,13 @@ virDomainHostdevDefFormat(virBufferPtr buf, } +/* The set of public flags we support for domainGetXMLDesc */ +#define DOMAIN_GET_XML_DESC_FLAGS \ + (VIR_DOMAIN_XML_SECURE | \ + VIR_DOMAIN_XML_INACTIVE | \ + VIR_DOMAIN_XML_UPDATE_CPU) +verify((VIR_DOMAIN_XML_INTERNAL_STATUS & DOMAIN_GET_XML_DESC_FLAGS) == 0); + char *virDomainDefFormat(virDomainDefPtr def, unsigned int flags) { @@ -9624,9 +9637,7 @@ char *virDomainDefFormat(virDomainDefPtr def, const char *type = NULL; int n, allones = 1; - virCheckFlags(VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_UPDATE_CPU, NULL); + virCheckFlags(DOMAIN_GET_XML_DESC_FLAGS, NULL); if (!(type = virDomainVirtTypeToString(def->virtType))) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 172d3c2..7a3d72b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -41,11 +41,6 @@ # include "macvtap.h" # include "sysinfo.h" -/* Private component of virDomainXMLFlags */ -typedef enum { - VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ -} virDomainXMLInternalFlags; - /* Different types of hypervisor */ /* NB: Keep in sync with virDomainVirtTypeToString impl */ enum virDomainVirtType { @@ -1417,11 +1412,6 @@ virDomainDefPtr virDomainDefParseNode(virCapsPtr caps, unsigned int expectedVirtTypes, unsigned int flags); -virDomainObjPtr virDomainObjParseFile(virCapsPtr caps, - const char *filename, - unsigned int expectedVirtTypes, - unsigned int flags); - bool virDomainDefCheckABIStability(virDomainDefPtr src, virDomainDefPtr dst); diff --git a/src/driver.c b/src/driver.c index 579c2b3..5034277 100644 --- a/src/driver.c +++ b/src/driver.c @@ -32,10 +32,6 @@ #define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/connection-driver" -/* Make sure ... INTERNAL_CALL cannot be set by the caller */ -verify((VIR_SECRET_GET_VALUE_INTERNAL_CALL & - VIR_SECRET_GET_VALUE_FLAGS_MASK) == 0); - #ifdef WITH_DRIVER_MODULES /* XXX re-implment this for other OS, or use libtools helper lib ? */ diff --git a/src/driver.h b/src/driver.h index 70ea4c2..0e0b808 100644 --- a/src/driver.h +++ b/src/driver.h @@ -191,7 +191,7 @@ typedef char * unsigned int flags); typedef char * (*virDrvDomainGetXMLDesc) (virDomainPtr dom, - unsigned int flags); + unsigned int flags); typedef char * (*virDrvConnectDomainXMLFromNative) (virConnectPtr conn, const char *nativeFormat, @@ -1229,16 +1229,10 @@ struct _virDeviceMonitor { virDrvNodeDeviceDestroy deviceDestroy; }; -/* bits 16 and above of virDomainXMLFlags are for internal use */ -# define VIR_DOMAIN_XML_FLAGS_MASK 0xffff - -/* Bits 16 and above of virSecretGetValue flags are for internal use */ -# define VIR_SECRET_GET_VALUE_FLAGS_MASK 0xffff - enum { /* This getValue call is inside libvirt, override the "private" flag. This flag cannot be set by outside callers. */ - VIR_SECRET_GET_VALUE_INTERNAL_CALL = 1 << 16 + VIR_SECRET_GET_VALUE_INTERNAL_CALL = 1 << 0, }; typedef virSecretPtr @@ -1263,7 +1257,8 @@ typedef int typedef unsigned char * (*virDrvSecretGetValue) (virSecretPtr secret, size_t *value_size, - unsigned int flags); + unsigned int flags, + unsigned int internalFlags); typedef int (*virDrvSecretUndefine) (virSecretPtr secret); typedef int diff --git a/src/libvirt.c b/src/libvirt.c index 7e70caa..bffb9e3 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3381,8 +3381,6 @@ virDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) goto error; } - flags &= VIR_DOMAIN_XML_FLAGS_MASK; - if (conn->driver->domainGetXMLDesc) { char *ret; ret = conn->driver->domainGetXMLDesc(domain, flags); @@ -12695,12 +12693,10 @@ virSecretGetValue(virSecretPtr secret, size_t *value_size, unsigned int flags) goto error; } - flags &= VIR_SECRET_GET_VALUE_FLAGS_MASK; - if (conn->secretDriver != NULL && conn->secretDriver->getValue != NULL) { unsigned char *ret; - ret = conn->secretDriver->getValue(secret, value_size, flags); + ret = conn->secretDriver->getValue(secret, value_size, flags, 0); if (ret == NULL) goto error; return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d0085e0..448b06e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -276,7 +276,7 @@ qemuProcessGetVolumeQcowPassphrase(virConnectPtr conn, enc->secrets[0]->uuid); if (secret == NULL) goto cleanup; - data = conn->secretDriver->getValue(secret, &size, + data = conn->secretDriver->getValue(secret, &size, 0, VIR_SECRET_GET_VALUE_INTERNAL_CALL); virUnrefSecret(secret); if (data == NULL) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2d5dc15..538c6aa 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3173,7 +3173,7 @@ remoteSecretClose (virConnectPtr conn) static unsigned char * remoteSecretGetValue (virSecretPtr secret, size_t *value_size, - unsigned int flags) + unsigned int flags, unsigned int internalFlags) { unsigned char *rv = NULL; remote_secret_get_value_args args; @@ -3182,6 +3182,12 @@ remoteSecretGetValue (virSecretPtr secret, size_t *value_size, remoteDriverLock (priv); + /* internalFlags intentionally do not go over the wire */ + if (internalFlags) { + remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no internalFlags support")); + goto done; + } + make_nonnull_secret (&args.secret, secret); args.flags = flags; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index c45ba51..02cdbb9 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -873,12 +873,15 @@ cleanup: } static unsigned char * -secretGetValue(virSecretPtr obj, size_t *value_size, unsigned int flags) +secretGetValue(virSecretPtr obj, size_t *value_size, unsigned int flags, + unsigned int internalFlags) { virSecretDriverStatePtr driver = obj->conn->secretPrivateData; unsigned char *ret = NULL; virSecretEntryPtr secret; + virCheckFlags(0, NULL); + secretDriverLock(driver); secret = secretFindByUUID(driver, obj->uuid); @@ -898,7 +901,7 @@ secretGetValue(virSecretPtr obj, size_t *value_size, unsigned int flags) goto cleanup; } - if ((flags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && + if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && secret->def->private) { virSecretReportError(VIR_ERR_OPERATION_DENIED, "%s", _("secret is private")); -- 1.7.4.4
Enforce the recent flags cleanups - we want to use 'unsigned int flags' in any of our APIs (except where backwards compatibility is important, in the public migration APIs), and that all flags are checked for validity (except when there are stub functions that completely ignore the flags argument). There are a few minor tweaks done here to avoid false positives: signed arguments passed to open() are renamed oflags, and flags arguments that are legitimately ignored are renamed flags_unused. * cfg.mk (sc_flags_usage): New rule. (exclude_file_name_regexp--sc_flags_usage): And a few exemptions. (sc_flags_debug): Tweak wording. * src/util/iohelper.c (runIO, main): Rename variable. * src/util/util.c (virSetInherit): Likewise. * src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile): Likewise. * src/fdstream.c (virFDStreamOpenFileInternal) (virFDStreamOpenFile, virFDStreamCreateFile): Likewise. * src/util/command.c (virExecWithHook) [WIN32]: Likewise. * src/util/util.c (virFileOpenAs, virDirCreate) [WIN32]: Likewise. * src/locking/lock_manager.c (virLockManagerPluginNew) [!HAVE_DLFCN_H]: Likewise. * src/locking/lock_driver_nop.c (virLockManagerNopNew) (virLockManagerNopAddResource, virLockManagerNopAcquire) (virLockManagerNopRelease, virLockManagerNopInquire): Likewise. --- v3: minor tweaks to make syntax check rules catch a few more cases cfg.mk | 30 ++++++++++++++++++++++++++---- src/fdstream.c | 28 ++++++++++++++-------------- src/fdstream.h | 6 +++--- src/locking/lock_driver_nop.c | 10 +++++----- src/locking/lock_manager.c | 7 ++++--- src/util/command.c | 2 +- src/util/iohelper.c | 18 +++++++++--------- src/util/util.c | 14 +++++++------- 8 files changed, 69 insertions(+), 46 deletions(-) diff --git a/cfg.mk b/cfg.mk index 7c3e2d2..60779eb 100644 --- a/cfg.mk +++ b/cfg.mk @@ -273,10 +273,32 @@ sc_avoid_write: # In debug statements, print flags as bitmask and mode_t as octal. sc_flags_debug: @prohibit='\<mode=%[0-9.]*[diux]' \ - halt='debug mode_t values with %o' \ + halt='use %o to debug mode_t values' \ $(_sc_search_regexp) - @prohibit='\<flags=%[0-9.]*l*[diou]' \ - halt='debug flag values with %x' \ + @prohibit='[Ff]lags=%[0-9.]*l*[diou]' \ + halt='use %x to debug flag values' \ + $(_sc_search_regexp) + +# Prefer 'unsigned int flags', along with checks for unknown flags. +# For historical reasons, we are stuck with 'unsigned long flags' in +# migration, so check for those known 4 instances and no more in public +# API. Also check that no flags are marked unused, and 'unsigned' should +# appear before any declaration of a flags variable (acheived by +# prohibiting the word prior to the type from ending in anything other +# than d). The existence of long long, and of documentation about +# flags, makes the regex in the third test slightly harder. +sc_flags_usage: + @test "$$(cat $(srcdir)/include/libvirt/libvirt.h.in \ + $(srcdir)/include/libvirt/virterror.h \ + $(srcdir)/include/libvirt/libvirt-qemu.h \ + | grep -c '\(long\|unsigned\) flags')" != 4 && \ + { echo '$(ME): new API should use "unsigned int flags"' 1>&2; \ + exit 1; } || : + @prohibit=' flags ''ATTRIBUTE_UNUSED' \ + halt='flags should be checked with virCheckFlags' \ + $(_sc_search_regexp) + @prohibit='^[^@]*([^d] (int|long long)|[^dg] long) flags[;,)]' \ + halt='flags should be unsigned' \ $(_sc_search_regexp) # Avoid functions that can lead to double-close bugs. @@ -645,7 +667,7 @@ exclude_file_name_regexp--sc_avoid_write = \ exclude_file_name_regexp--sc_bindtextdomain = ^(tests|examples)/ -exclude_file_name_regexp--sc_flags_debug = ^docs/ +exclude_file_name_regexp--sc_flags_usage = ^docs/ exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^src/rpc/gendispatch\.pl$$ diff --git a/src/fdstream.c b/src/fdstream.c index 4dbe4a3..dd742e1 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -504,7 +504,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int flags, + int oflags, int mode, bool delete) { @@ -514,13 +514,13 @@ virFDStreamOpenFileInternal(virStreamPtr st, virCommandPtr cmd = NULL; int errfd = -1; - VIR_DEBUG("st=%p path=%s flags=%x offset=%llu length=%llu mode=%o delete=%d", - st, path, flags, offset, length, mode, delete); + VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o delete=%d", + st, path, oflags, offset, length, mode, delete); - if (flags & O_CREAT) - fd = open(path, flags, mode); + if (oflags & O_CREAT) + fd = open(path, oflags, mode); else - fd = open(path, flags); + fd = open(path, oflags); if (fd < 0) { virReportSystemError(errno, _("Unable to open stream for '%s'"), @@ -545,7 +545,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, !S_ISFIFO(sb.st_mode))) { int childfd; - if ((flags & O_RDWR) == O_RDWR) { + if ((oflags & O_RDWR) == O_RDWR) { streamsReportError(VIR_ERR_INTERNAL_ERROR, _("%s: Cannot request read and write flags together"), path); @@ -562,7 +562,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", path, NULL); - virCommandAddArgFormat(cmd, "%d", flags); + virCommandAddArgFormat(cmd, "%d", oflags); virCommandAddArgFormat(cmd, "%d", mode); virCommandAddArgFormat(cmd, "%llu", offset); virCommandAddArgFormat(cmd, "%llu", length); @@ -575,7 +575,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, */ delete = false; - if (flags == O_RDONLY) { + if (oflags == O_RDONLY) { childfd = fds[1]; fd = fds[0]; virCommandSetOutputFD(cmd, &childfd); @@ -620,10 +620,10 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int flags, + int oflags, bool delete) { - if (flags & O_CREAT) { + if (oflags & O_CREAT) { streamsReportError(VIR_ERR_INTERNAL_ERROR, _("Attempt to create %s without specifying mode"), path); @@ -631,19 +631,19 @@ int virFDStreamOpenFile(virStreamPtr st, } return virFDStreamOpenFileInternal(st, path, offset, length, - flags, 0, delete); + oflags, 0, delete); } int virFDStreamCreateFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int flags, + int oflags, mode_t mode, bool delete) { return virFDStreamOpenFileInternal(st, path, offset, length, - flags | O_CREAT, + oflags | O_CREAT, mode, delete); } diff --git a/src/fdstream.h b/src/fdstream.h index a66902b..76f0cd0 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -1,7 +1,7 @@ /* * fdstream.h: generic streams impl for file descriptors * - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -37,13 +37,13 @@ int virFDStreamOpenFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int flags, + int oflags, bool delete); int virFDStreamCreateFile(virStreamPtr st, const char *path, unsigned long long offset, unsigned long long length, - int flags, + int oflags, mode_t mode, bool delete); diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c index 0dcd794..4f35afa 100644 --- a/src/locking/lock_driver_nop.c +++ b/src/locking/lock_driver_nop.c @@ -49,7 +49,7 @@ static int virLockManagerNopNew(virLockManagerPtr lock ATTRIBUTE_UNUSED, unsigned int type ATTRIBUTE_UNUSED, size_t nparams ATTRIBUTE_UNUSED, virLockManagerParamPtr params ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags_unused ATTRIBUTE_UNUSED) { return 0; } @@ -59,7 +59,7 @@ static int virLockManagerNopAddResource(virLockManagerPtr lock ATTRIBUTE_UNUSED, const char *name ATTRIBUTE_UNUSED, size_t nparams ATTRIBUTE_UNUSED, virLockManagerParamPtr params ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags_unused ATTRIBUTE_UNUSED) { return 0; @@ -68,7 +68,7 @@ static int virLockManagerNopAddResource(virLockManagerPtr lock ATTRIBUTE_UNUSED, static int virLockManagerNopAcquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, const char *state ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED, + unsigned int flags_unused ATTRIBUTE_UNUSED, int *fd ATTRIBUTE_UNUSED) { return 0; @@ -76,7 +76,7 @@ static int virLockManagerNopAcquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, static int virLockManagerNopRelease(virLockManagerPtr lock ATTRIBUTE_UNUSED, char **state, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags_unused ATTRIBUTE_UNUSED) { if (state) *state = NULL; @@ -86,7 +86,7 @@ static int virLockManagerNopRelease(virLockManagerPtr lock ATTRIBUTE_UNUSED, static int virLockManagerNopInquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, char **state, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags_unused ATTRIBUTE_UNUSED) { if (state) *state = NULL; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index d27cf8f..f07f9d7 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -190,9 +190,10 @@ cleanup: return NULL; } #else /* !HAVE_DLFCN_H */ -virLockManagerPluginPtr virLockManagerPluginNew(const char *name ATTRIBUTE_UNUSED, - const char *configFile ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) +virLockManagerPluginPtr +virLockManagerPluginNew(const char *name ATTRIBUTE_UNUSED, + const char *configFile ATTRIBUTE_UNUSED, + unsigned int flags_unused ATTRIBUTE_UNUSED) { virLockError(VIR_ERR_INTERNAL_ERROR, "%s", _("this platform is missing dlopen")); diff --git a/src/util/command.c b/src/util/command.c index f3c35ed..11dd7b0 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -622,7 +622,7 @@ virExecWithHook(const char *const*argv ATTRIBUTE_UNUSED, int infd ATTRIBUTE_UNUSED, int *outfd ATTRIBUTE_UNUSED, int *errfd ATTRIBUTE_UNUSED, - int flags ATTRIBUTE_UNUSED, + int flags_unused ATTRIBUTE_UNUSED, virExecHook hook ATTRIBUTE_UNUSED, void *data ATTRIBUTE_UNUSED, char *pidfile ATTRIBUTE_UNUSED) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index f519d5a..0368eba 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -43,7 +43,7 @@ #define VIR_FROM_THIS VIR_FROM_STORAGE static int runIO(const char *path, - int flags, + int oflags, int mode, unsigned long long offset, unsigned long long length) @@ -56,10 +56,10 @@ static int runIO(const char *path, const char *fdinname, *fdoutname; unsigned long long total = 0; - if (flags & O_CREAT) { - fd = open(path, flags, mode); + if (oflags & O_CREAT) { + fd = open(path, oflags, mode); } else { - fd = open(path, flags); + fd = open(path, oflags); } if (fd < 0) { virReportSystemError(errno, _("Unable to open %s"), path); @@ -79,7 +79,7 @@ static int runIO(const char *path, goto cleanup; } - switch (flags & O_ACCMODE) { + switch (oflags & O_ACCMODE) { case O_RDONLY: fdin = fd; fdinname = path; @@ -97,7 +97,7 @@ static int runIO(const char *path, default: virReportSystemError(EINVAL, _("Unable to process file with flags %d"), - (flags & O_ACCMODE)); + (oflags & O_ACCMODE)); goto cleanup; } @@ -144,7 +144,7 @@ int main(int argc, char **argv) virErrorPtr err; unsigned long long offset; unsigned long long length; - int flags; + int oflags; int mode; unsigned int delete; @@ -169,7 +169,7 @@ int main(int argc, char **argv) path = argv[1]; - if (virStrToLong_i(argv[2], NULL, 10, &flags) < 0) { + if (virStrToLong_i(argv[2], NULL, 10, &oflags) < 0) { fprintf(stderr, _("%s: malformed file flags %s"), argv[0], argv[2]); exit(EXIT_FAILURE); } @@ -192,7 +192,7 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } - if (runIO(path, flags, mode, offset, length) < 0) + if (runIO(path, oflags, mode, offset, length) < 0) goto error; if (delete) diff --git a/src/util/util.c b/src/util/util.c index 0ca81df..910bb04 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -252,14 +252,14 @@ virArgvToString(const char *const *argv) #ifndef WIN32 int virSetInherit(int fd, bool inherit) { - int flags; - if ((flags = fcntl(fd, F_GETFD)) < 0) + int fflags; + if ((fflags = fcntl(fd, F_GETFD)) < 0) return -1; if (inherit) - flags &= ~FD_CLOEXEC; + fflags &= ~FD_CLOEXEC; else - flags |= FD_CLOEXEC; - if ((fcntl(fd, F_SETFD, flags)) < 0) + fflags |= FD_CLOEXEC; + if ((fcntl(fd, F_SETFD, fflags)) < 0) return -1; return 0; } @@ -981,7 +981,7 @@ int virFileOpenAs(const char *path ATTRIBUTE_UNUSED, mode_t mode ATTRIBUTE_UNUSED, uid_t uid ATTRIBUTE_UNUSED, gid_t gid ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags_unused ATTRIBUTE_UNUSED) { virUtilError(VIR_ERR_INTERNAL_ERROR, "%s", _("virFileOpenAs is not implemented for WIN32")); @@ -993,7 +993,7 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED, mode_t mode ATTRIBUTE_UNUSED, uid_t uid ATTRIBUTE_UNUSED, gid_t gid ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags_unused ATTRIBUTE_UNUSED) { virUtilError(VIR_ERR_INTERNAL_ERROR, "%s", _("virDirCreate is not implemented for WIN32")); -- 1.7.4.4
2011/7/15 Eric Blake <eblake@redhat.com>:
Enforce the recent flags cleanups - we want to use 'unsigned int flags' in any of our APIs (except where backwards compatibility is important, in the public migration APIs), and that all flags are checked for validity (except when there are stub functions that completely ignore the flags argument).
There are a few minor tweaks done here to avoid false positives: signed arguments passed to open() are renamed oflags, and flags arguments that are legitimately ignored are renamed flags_unused.
* cfg.mk (sc_flags_usage): New rule. (exclude_file_name_regexp--sc_flags_usage): And a few exemptions. (sc_flags_debug): Tweak wording. * src/util/iohelper.c (runIO, main): Rename variable. * src/util/util.c (virSetInherit): Likewise. * src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile): Likewise. * src/fdstream.c (virFDStreamOpenFileInternal) (virFDStreamOpenFile, virFDStreamCreateFile): Likewise. * src/util/command.c (virExecWithHook) [WIN32]: Likewise. * src/util/util.c (virFileOpenAs, virDirCreate) [WIN32]: Likewise. * src/locking/lock_manager.c (virLockManagerPluginNew) [!HAVE_DLFCN_H]: Likewise. * src/locking/lock_driver_nop.c (virLockManagerNopNew) (virLockManagerNopAddResource, virLockManagerNopAcquire) (virLockManagerNopRelease, virLockManagerNopInquire): Likewise. ---
v3: minor tweaks to make syntax check rules catch a few more cases
cfg.mk | 30 ++++++++++++++++++++++++++---- src/fdstream.c | 28 ++++++++++++++-------------- src/fdstream.h | 6 +++--- src/locking/lock_driver_nop.c | 10 +++++----- src/locking/lock_manager.c | 7 ++++--- src/util/command.c | 2 +- src/util/iohelper.c | 18 +++++++++--------- src/util/util.c | 14 +++++++------- 8 files changed, 69 insertions(+), 46 deletions(-)
ACK. -- Matthias Bolte http://photron.blogspot.com
On 07/15/2011 09:52 AM, Matthias Bolte wrote:
2011/7/15 Eric Blake<eblake@redhat.com>:
Enforce the recent flags cleanups - we want to use 'unsigned int flags' in any of our APIs (except where backwards compatibility is important, in the public migration APIs), and that all flags are checked for validity (except when there are stub functions that completely ignore the flags argument).
There are a few minor tweaks done here to avoid false positives: signed arguments passed to open() are renamed oflags, and flags arguments that are legitimately ignored are renamed flags_unused.
ACK.
I've pushed this now. I'll post a v4 of patch 5/6, which picks up some additional places where libvirt.c is wrongly filtering flags. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Matthias Bolte