[libvirt] [PATCH v2 0/2] Enhance checking of supported flags in drivers
I think we need a simple and consistent way of checking whether flags passed to API entry points in drivers are valid/supported or not. So far some entry points don't check flags at all and some checks for 0 when no flags are expected/supported. The first patch introduces a macro which can be called with a whitelist of supported flags (more info about it inside the patch). The second patch changes all API calls introduced in 0.8.0 release to use the macro for checking valid flags. I'm not sure if we can change existing API calls to do the same as it could break some badly written apps which pass invalid flags. My opinion is that we may often do that as such apps don't use the API in a documented/valid way. Changes in v2: - virCheckFlags moved from checks.h to existing internal.h Jirka Jiri Denemark (2): Introduce virCheckFlags for consistent flags checking Use virCheckFlags for APIs added in 0.8.0 src/esx/esx_driver.c | 43 ++++++++++++------------- src/internal.h | 23 +++++++++++++ src/nwfilter/nwfilter_driver.c | 4 ++- src/qemu/qemu_driver.c | 68 +++++++++++++++++++--------------------- src/storage/storage_driver.c | 6 +--- src/vbox/vbox_tmpl.c | 41 ++++++++++++++++------- src/xen/xend_internal.c | 4 ++ 7 files changed, 111 insertions(+), 78 deletions(-)
The idea is that every API implementation in driver which has flags parameter should first call virCheckFlags() macro to check the function was called with supported flags: virCheckFlags(VIR_SUPPORTED_FLAG_1 | VIR_SUPPORTED_FLAG_2 | VIR_ANOTHER_SUPPORTED_FLAG, -1); The error massage which is printed when unsupported flags are passed looks like: invalid argument in virFooBar: unsupported flags (0x2) Where the unsupported flags part only prints those flags which were passed but are not supported rather than all flags passed. --- src/internal.h | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-) diff --git a/src/internal.h b/src/internal.h index 2e73210..12e9a1f 100644 --- a/src/internal.h +++ b/src/internal.h @@ -191,4 +191,27 @@ fprintf(stderr, "Unimplemented block at %s:%d\n", \ __FILE__, __LINE__); +/** + * virCheckFlags: + * @supported: an OR'ed set of supported flags + * @retval: return value in case unsupported flags were passed + * + * Returns nothing. Exits the caller function if unsupported flags were + * passed to it. + */ +# define virCheckFlags(supported, retval) \ + do { \ + if ((flags & ~(supported))) { \ + virReportErrorHelper(NULL, \ + VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, \ + __FUNCTION__, \ + __LINE__, \ + _("%s: unsupported flags (0x%x)"), \ + __FUNCTION__, flags & ~(supported)); \ + return retval; \ + } \ + } while (0) + #endif /* __VIR_INTERNAL_H__ */ -- 1.7.0.4
On 04/13/2010 10:25 AM, Jiri Denemark wrote:
The idea is that every API implementation in driver which has flags parameter should first call virCheckFlags() macro to check the function was called with supported flags:
virCheckFlags(VIR_SUPPORTED_FLAG_1 | VIR_SUPPORTED_FLAG_2 | VIR_ANOTHER_SUPPORTED_FLAG, -1);
The error massage which is printed when unsupported flags are passed looks like:
invalid argument in virFooBar: unsupported flags (0x2)
Where the unsupported flags part only prints those flags which were passed but are not supported rather than all flags passed. --- src/internal.h | 23 +++++++++++++++++++++++
I like this location better than the v1 attempt. ACK (but note my comments on Matthias' ESX patch, depending on which gets pushed first).
+/** + * virCheckFlags: + * @supported: an OR'ed set of supported flags
Is it worth documenting that this must be 'int' or 'unsigned int', and that the macro doesn't work on uint64_t? Or maybe it's worth trying to figure out a way to refactor the macro to support both sizes? But that can be a followup, if we find a case where we ever need a larger size for flags, so it shouldn't hold up committing this. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The idea is that every API implementation in driver which has flags parameter should first call virCheckFlags() macro to check the function was called with supported flags:
virCheckFlags(VIR_SUPPORTED_FLAG_1 | VIR_SUPPORTED_FLAG_2 | VIR_ANOTHER_SUPPORTED_FLAG, -1);
The error massage which is printed when unsupported flags are passed looks like:
invalid argument in virFooBar: unsupported flags (0x2)
Where the unsupported flags part only prints those flags which were passed but are not supported rather than all flags passed. --- src/internal.h | 23 +++++++++++++++++++++++
I like this location better than the v1 attempt. ACK (but note my comments on Matthias' ESX patch, depending on which gets pushed first).
Thanks, I pushed the patch.
+/** + * virCheckFlags: + * @supported: an OR'ed set of supported flags
Is it worth documenting that this must be 'int' or 'unsigned int', and that the macro doesn't work on uint64_t? Or maybe it's worth trying to figure out a way to refactor the macro to support both sizes? But that can be a followup, if we find a case where we ever need a larger size for flags, so it shouldn't hold up committing this.
I left it as is for now as we only use (unsigned) int flags now. Once we add an API with 64b flags we can enhance this macro. Jirka
--- src/esx/esx_driver.c | 43 ++++++++++++------------- src/nwfilter/nwfilter_driver.c | 4 ++- src/qemu/qemu_driver.c | 68 +++++++++++++++++++--------------------- src/storage/storage_driver.c | 6 +--- src/vbox/vbox_tmpl.c | 41 ++++++++++++++++------- src/xen/xend_internal.c | 4 ++ 6 files changed, 88 insertions(+), 78 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4ed9890..d9a1cfd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3296,7 +3296,7 @@ esxDomainIsPersistent(virDomainPtr domain ATTRIBUTE_UNUSED) static virDomainSnapshotPtr esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = domain->conn->privateData; virDomainSnapshotDefPtr def = NULL; @@ -3308,6 +3308,8 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, esxVI_TaskInfoState taskInfoState; virDomainSnapshotPtr snapshot = NULL; + virCheckFlags(0, NULL); + if (esxVI_EnsureSession(priv->host) < 0) { goto failure; } @@ -3369,7 +3371,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, static char * esxDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = snapshot->domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotList = NULL; @@ -3379,6 +3381,8 @@ esxDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, char uuid_string[VIR_UUID_STRING_BUFLEN] = ""; char *xml = NULL; + virCheckFlags(0, NULL); + memset(&def, 0, sizeof (virDomainSnapshotDef)); if (esxVI_EnsureSession(priv->host) < 0) { @@ -3423,12 +3427,14 @@ esxDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, static int -esxDomainSnapshotNum(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED) +esxDomainSnapshotNum(virDomainPtr domain, unsigned int flags) { int result = 0; esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; + virCheckFlags(0, -1); + if (esxVI_EnsureSession(priv->host) < 0) { goto failure; } @@ -3455,12 +3461,14 @@ esxDomainSnapshotNum(virDomainPtr domain, unsigned int flags ATTRIBUTE_UNUSED) static int esxDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { int result = 0; esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; + virCheckFlags(0, -1); + if (names == NULL || nameslen < 0) { ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); return -1; @@ -3496,7 +3504,7 @@ esxDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, static virDomainSnapshotPtr esxDomainSnapshotLookupByName(virDomainPtr domain, const char *name, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; @@ -3504,6 +3512,8 @@ esxDomainSnapshotLookupByName(virDomainPtr domain, const char *name, esxVI_VirtualMachineSnapshotTree *snapshotTreeParent = NULL; virDomainSnapshotPtr snapshot = NULL; + virCheckFlags(0, NULL); + if (esxVI_EnsureSession(priv->host) < 0) { goto cleanup; } @@ -3533,12 +3543,7 @@ esxDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *currentSnapshotTree = NULL; - if (flags != 0) { - ESX_ERROR(VIR_ERR_INVALID_ARG, - _("Unsupported flags (0x%x) passed to %s"), - flags, __FUNCTION__); - return -1; - } + virCheckFlags(0, -1); if (esxVI_EnsureSession(priv->host) < 0) { goto failure; @@ -3574,12 +3579,7 @@ esxDomainSnapshotCurrent(virDomainPtr domain, unsigned int flags) esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *currentSnapshotTree = NULL; - if (flags != 0) { - ESX_ERROR(VIR_ERR_INVALID_ARG, - _("Unsupported flags (0x%x) passed to %s"), - flags, __FUNCTION__); - return NULL; - } + virCheckFlags(0, NULL); if (esxVI_EnsureSession(priv->host) < 0) { goto cleanup; @@ -3612,12 +3612,7 @@ esxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) esxVI_ManagedObjectReference *task = NULL; esxVI_TaskInfoState taskInfoState; - if (flags != 0) { - ESX_ERROR(VIR_ERR_INVALID_ARG, - _("Unsupported flags (0x%x) passed to %s"), - flags, __FUNCTION__); - return -1; - } + virCheckFlags(0, -1); if (esxVI_EnsureSession(priv->host) < 0) { goto failure; @@ -3670,6 +3665,8 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) esxVI_ManagedObjectReference *task = NULL; esxVI_TaskInfoState taskInfoState; + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + if (esxVI_EnsureSession(priv->host) < 0) { goto failure; } diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index f237b7c..35ce807 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -374,11 +374,13 @@ cleanup: static char * nwfilterDumpXML(virNWFilterPtr obj, - unsigned int flags ATTRIBUTE_UNUSED) { + unsigned int flags) { virNWFilterDriverStatePtr driver = obj->conn->nwfilterPrivateData; virNWFilterPoolObjPtr pool; char *ret = NULL; + virCheckFlags(0, NULL); + nwfilterDriverLock(driver); pool = virNWFilterPoolObjFindByUUID(&driver->pools, obj->uuid); nwfilterDriverUnlock(driver); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index df1d435..3981a8d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4997,12 +4997,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) int ret = -1; int compressed; - if (flags != 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("unsupported flags (0x%x) passed to '%s'"), - flags, __FUNCTION__); - return -1; - } + virCheckFlags(0, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5063,12 +5058,7 @@ qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) int ret = -1; char *name = NULL; - if (flags != 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("unsupported flags (0x%x) passed to '%s'"), - flags, __FUNCTION__); - return -1; - } + virCheckFlags(0, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5102,12 +5092,7 @@ qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) int ret = -1; char *name = NULL; - if (flags != 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("unsupported flags (0x%x) passed to '%s'"), - flags, __FUNCTION__); - return -1; - } + virCheckFlags(0, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -7630,11 +7615,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, virCgroupPtr cgroup = NULL; int ret = -1; - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); - return -1; - } + virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_CURRENT | + VIR_DOMAIN_DEVICE_MODIFY_LIVE, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -10516,11 +10498,7 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom, qemuDomainObjPrivatePtr priv; int ret = -1; - if (flags != 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("unsupported flags (0x%x) passed to '%s'"), flags, __FUNCTION__); - return -1; - } + virCheckFlags(0, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -10679,7 +10657,7 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm = NULL; @@ -10691,6 +10669,8 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL }; int i; + virCheckFlags(0, NULL); + qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -10796,12 +10776,14 @@ cleanup: static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm = NULL; int n = -1; + virCheckFlags(0, -1); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); if (!vm) { @@ -10822,12 +10804,14 @@ cleanup: } static int qemuDomainSnapshotNum(virDomainPtr domain, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm = NULL; int n = -1; + virCheckFlags(0, -1); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); if (!vm) { @@ -10849,13 +10833,15 @@ cleanup: static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, const char *name, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; + virCheckFlags(0, NULL); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); if (!vm) { @@ -10883,12 +10869,14 @@ cleanup: } static int qemuDomainHasCurrentSnapshot(virDomainPtr domain, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm; int ret = -1; + virCheckFlags(0, -1); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); if (!vm) { @@ -10909,12 +10897,14 @@ cleanup: } static virDomainSnapshotPtr qemuDomainSnapshotCurrent(virDomainPtr domain, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm; virDomainSnapshotPtr snapshot = NULL; + virCheckFlags(0, NULL); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); if (!vm) { @@ -10941,7 +10931,7 @@ cleanup: } static char *qemuDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = snapshot->domain->conn->privateData; virDomainObjPtr vm = NULL; @@ -10949,6 +10939,8 @@ static char *qemuDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virCheckFlags(0, NULL); + qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); @@ -10976,7 +10968,7 @@ cleanup: } static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct qemud_driver *driver = snapshot->domain->conn->privateData; virDomainObjPtr vm = NULL; @@ -10987,6 +10979,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; + virCheckFlags(0, -1); + qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); @@ -11212,6 +11206,8 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, char uuidstr[VIR_UUID_STRING_BUFLEN]; struct snap_remove rem; + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d476822..2c69ba9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1681,11 +1681,7 @@ storageVolumeWipe(virStorageVolPtr obj, virStorageVolDefPtr vol = NULL; int ret = -1; - if (flags != 0) { - virStorageReportError(VIR_ERR_INVALID_ARG, - _("Unsupported flags (0x%x) passed to '%s'"), flags, __FUNCTION__); - goto out; - } + virCheckFlags(0, -1); storageDriverLock(driver); pool = virStoragePoolObjFindByName(&driver->pools, obj->pool); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index a07cf8e..0ea99d1 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -4877,11 +4877,8 @@ static int vboxDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, static int vboxDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - vboxError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify the persistent configuration of a domain")); - return -1; - } + virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_CURRENT | + VIR_DOMAIN_DEVICE_MODIFY_LIVE, -1); return vboxDomainAttachDeviceImpl(dom, xml, 1); } @@ -5166,7 +5163,7 @@ cleanup: static virDomainSnapshotPtr vboxDomainSnapshotCreateXML(virDomainPtr dom, const char *xmlDesc, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, virDomainSnapshotPtr, NULL); virDomainSnapshotDefPtr def = NULL; @@ -5185,6 +5182,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, PRInt32 result; #endif + virCheckFlags(0, NULL); + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) goto cleanup; @@ -5282,7 +5281,7 @@ cleanup: static char * vboxDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { virDomainPtr dom = snapshot->domain; VBOX_OBJECT_CHECK(dom->conn, char *, NULL); @@ -5298,6 +5297,8 @@ vboxDomainSnapshotDumpXML(virDomainSnapshotPtr snapshot, PRBool online = PR_FALSE; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virCheckFlags(0, NULL); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(domiid) < 0) { virReportOOMError(); @@ -5399,7 +5400,7 @@ no_memory: static int vboxDomainSnapshotNum(virDomainPtr dom, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, int, -1); vboxIID *iid = NULL; @@ -5407,6 +5408,8 @@ vboxDomainSnapshotNum(virDomainPtr dom, nsresult rc; PRUint32 snapshotCount; + virCheckFlags(0, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(); @@ -5442,7 +5445,7 @@ static int vboxDomainSnapshotListNames(virDomainPtr dom, char **names, int nameslen, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, int, -1); vboxIID *iid = NULL; @@ -5452,6 +5455,8 @@ vboxDomainSnapshotListNames(virDomainPtr dom, int count = 0; int i; + virCheckFlags(0, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(); @@ -5512,7 +5517,7 @@ cleanup: static virDomainSnapshotPtr vboxDomainSnapshotLookupByName(virDomainPtr dom, const char *name, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, virDomainSnapshotPtr, NULL); vboxIID *iid = NULL; @@ -5520,6 +5525,8 @@ vboxDomainSnapshotLookupByName(virDomainPtr dom, ISnapshot *snapshot = NULL; nsresult rc; + virCheckFlags(0, NULL); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(); @@ -5549,7 +5556,7 @@ cleanup: static int vboxDomainHasCurrentSnapshot(virDomainPtr dom, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, int, -1); vboxIID *iid = NULL; @@ -5557,6 +5564,8 @@ vboxDomainHasCurrentSnapshot(virDomainPtr dom, ISnapshot *snapshot = NULL; nsresult rc; + virCheckFlags(0, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(); @@ -5592,7 +5601,7 @@ cleanup: static virDomainSnapshotPtr vboxDomainSnapshotCurrent(virDomainPtr dom, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, virDomainSnapshotPtr, NULL); vboxIID *iid = NULL; @@ -5602,6 +5611,8 @@ vboxDomainSnapshotCurrent(virDomainPtr dom, char *name = NULL; nsresult rc; + virCheckFlags(0, NULL); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(); @@ -5763,7 +5774,7 @@ cleanup: static int vboxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { virDomainPtr dom = snapshot->domain; VBOX_OBJECT_CHECK(dom->conn, int, -1); @@ -5775,6 +5786,8 @@ vboxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, PRUint32 state; nsresult rc; + virCheckFlags(0, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(domiid) < 0) { virReportOOMError(); @@ -5941,6 +5954,8 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, PRUint32 state; nsresult rc; + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(domiid) < 0) { virReportOOMError(); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index c4e73b7..d81f6e3 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -4202,6 +4202,10 @@ 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 | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1); + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; -- 1.7.0.4
On 04/13/2010 10:25 AM, Jiri Denemark wrote:
--- src/esx/esx_driver.c | 43 ++++++++++++------------- src/nwfilter/nwfilter_driver.c | 4 ++- src/qemu/qemu_driver.c | 68 +++++++++++++++++++--------------------- src/storage/storage_driver.c | 6 +--- src/vbox/vbox_tmpl.c | 41 ++++++++++++++++------- src/xen/xend_internal.c | 4 ++ 6 files changed, 88 insertions(+), 78 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4ed9890..d9a1cfd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3296,7 +3296,7 @@ esxDomainIsPersistent(virDomainPtr domain ATTRIBUTE_UNUSED)
static virDomainSnapshotPtr esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = domain->conn->privateData; virDomainSnapshotDefPtr def = NULL; @@ -3308,6 +3308,8 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, esxVI_TaskInfoState taskInfoState; virDomainSnapshotPtr snapshot = NULL;
+ virCheckFlags(0, NULL);
I scanned through the patch, and didn't see any instances where we are calling virCheckFlags after non-trivial work. It is something to be aware of when using the macro in the future, since: { ptr *foo = somethingThatMallocs(); virCheckFlags(0, NULL); would be a memory leak, masked because the return is hidden inside the virCheckFlags macro. But the alternative, of making virCheckFlags result in a bool, and making all callers use: if (virBadFlags(0)) return NULL; is not as aesthetic. So: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Apr 15, 2010 at 08:41:07AM -0600, Eric Blake wrote:
On 04/13/2010 10:25 AM, Jiri Denemark wrote:
--- src/esx/esx_driver.c | 43 ++++++++++++------------- src/nwfilter/nwfilter_driver.c | 4 ++- src/qemu/qemu_driver.c | 68 +++++++++++++++++++--------------------- src/storage/storage_driver.c | 6 +--- src/vbox/vbox_tmpl.c | 41 ++++++++++++++++------- src/xen/xend_internal.c | 4 ++ 6 files changed, 88 insertions(+), 78 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4ed9890..d9a1cfd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3296,7 +3296,7 @@ esxDomainIsPersistent(virDomainPtr domain ATTRIBUTE_UNUSED)
static virDomainSnapshotPtr esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { esxPrivate *priv = domain->conn->privateData; virDomainSnapshotDefPtr def = NULL; @@ -3308,6 +3308,8 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, esxVI_TaskInfoState taskInfoState; virDomainSnapshotPtr snapshot = NULL;
+ virCheckFlags(0, NULL);
I scanned through the patch, and didn't see any instances where we are calling virCheckFlags after non-trivial work. It is something to be aware of when using the macro in the future, since:
{ ptr *foo = somethingThatMallocs(); virCheckFlags(0, NULL);
would be a memory leak, masked because the return is hidden inside the virCheckFlags macro.
But the alternative, of making virCheckFlags result in a bool, and making all callers use:
if (virBadFlags(0)) return NULL;
is not as aesthetic. So:
ACK.
Agreed it's the right time, and internal.h is the right place for the macro, AC :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
I scanned through the patch, and didn't see any instances where we are calling virCheckFlags after non-trivial work. It is something to be aware of when using the macro in the future, since:
{ ptr *foo = somethingThatMallocs(); virCheckFlags(0, NULL);
would be a memory leak, masked because the return is hidden inside the virCheckFlags macro.
Yes. I added a note to macro documentation to make this issue more visible.
ACK.
And pushed, thanks. Jirka
participants (3)
-
Daniel Veillard -
Eric Blake -
Jiri Denemark