[libvirt] [PATCH 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). I don't particularily like introducing a new header file but non of existing header files looked like a good place to stick this macro in. If you think such a place exist, I'll be very happy to adapt the patch. The second patch changes all API calls introduced since the last 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. But even if my thinking is correct, we don't want to change it before 0.8.0. Jirka Jiri Denemark (2): Introduce virCheckFlags for consistent flags checking Use virCheckFlags for APIs added in 0.8.0 src/Makefile.am | 3 +- src/esx/esx_driver.c | 44 ++++++++++++------------- src/nwfilter/nwfilter_driver.c | 5 ++- src/qemu/qemu_driver.c | 69 +++++++++++++++++++--------------------- src/storage/storage_driver.c | 7 +--- src/util/checks.h | 37 +++++++++++++++++++++ src/vbox/vbox_tmpl.c | 42 +++++++++++++++++------- src/xen/xend_internal.c | 5 +++ 8 files changed, 133 insertions(+), 79 deletions(-) create mode 100644 src/util/checks.h
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_SUPPORTED_ANOTHER_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 not supported rather than all flags passed. --- src/Makefile.am | 3 ++- src/util/checks.h | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletions(-) create mode 100644 src/util/checks.h diff --git a/src/Makefile.am b/src/Makefile.am index d54e6d0..3db9d10 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -76,7 +76,8 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/xml.c util/xml.h \ - util/virterror.c util/virterror_internal.h + util/virterror.c util/virterror_internal.h \ + util/checks.h EXTRA_DIST += util/threads-pthread.c util/threads-win32.c diff --git a/src/util/checks.h b/src/util/checks.h new file mode 100644 index 0000000..2333856 --- /dev/null +++ b/src/util/checks.h @@ -0,0 +1,37 @@ +/* + * Linux block and network stats. + * + * Copyright (C) 2010 Red Hat, Inc. + * + * See COPYING.LIB for the License of this software + * + * Jiri Denemark <jdenemar@redhat.com> + */ + +#ifndef __CHECKS_H__ +# define __CHECKS_H__ + +# include "virterror_internal.h" + +/** + * 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 /* __CHECKS_H__ */ -- 1.7.0.4
--- src/esx/esx_driver.c | 44 ++++++++++++------------- src/nwfilter/nwfilter_driver.c | 5 ++- src/qemu/qemu_driver.c | 69 +++++++++++++++++++--------------------- src/storage/storage_driver.c | 7 +--- src/vbox/vbox_tmpl.c | 42 +++++++++++++++++------- src/xen/xend_internal.c | 5 +++ 6 files changed, 94 insertions(+), 78 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 4ed9890..026d9b5 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -33,6 +33,7 @@ #include "memory.h" #include "logging.h" #include "uuid.h" +#include "checks.h" #include "esx_driver.h" #include "esx_interface_driver.h" #include "esx_network_driver.h" @@ -3296,7 +3297,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 +3309,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 +3372,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 +3382,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 +3428,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 +3462,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 +3505,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 +3513,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 +3544,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 +3580,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 +3613,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 +3666,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 f366005..7ec3ebf 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -33,6 +33,7 @@ #include "datatypes.h" #include "memory.h" #include "domain_conf.h" +#include "checks.h" #include "nwfilter_driver.h" #include "nwfilter_gentech_driver.h" @@ -372,11 +373,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..aac169c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -85,6 +85,7 @@ #include "macvtap.h" #include "nwfilter/nwfilter_gentech_driver.h" #include "hooks.h" +#include "checks.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -4997,12 +4998,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 +5059,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 +5093,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 +7616,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 +10499,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 +10658,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 +10670,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 +10777,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 +10805,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 +10834,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 +10870,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 +10898,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 +10932,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 +10940,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 +10969,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 +10980,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 +11207,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..aca7f87 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -45,6 +45,7 @@ #include "memory.h" #include "storage_backend.h" #include "logging.h" +#include "checks.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1681,11 +1682,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..5a0100a 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -52,6 +52,7 @@ #include "memory.h" #include "nodeinfo.h" #include "logging.h" +#include "checks.h" #include "vbox_driver.h" /* This one changes from version to version. */ @@ -4877,11 +4878,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 +5164,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 +5183,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, PRInt32 result; #endif + virCheckFlags(0, NULL); + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) goto cleanup; @@ -5282,7 +5282,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 +5298,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 +5401,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 +5409,8 @@ vboxDomainSnapshotNum(virDomainPtr dom, nsresult rc; PRUint32 snapshotCount; + virCheckFlags(0, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(); @@ -5442,7 +5446,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 +5456,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 +5518,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 +5526,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 +5557,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 +5565,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 +5602,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 +5612,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 +5775,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 +5787,8 @@ vboxDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, PRUint32 state; nsresult rc; + virCheckFlags(0, -1); + #if VBOX_API_VERSION == 2002 if (VIR_ALLOC(domiid) < 0) { virReportOOMError(); @@ -5941,6 +5955,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..b16521c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -44,6 +44,7 @@ #include "xen_hypervisor.h" #include "xs_internal.h" /* To extract VNC port & Serial console TTY */ #include "memory.h" +#include "checks.h" /* required for cpumap_t */ #include <xen/dom0_ops.h> @@ -4202,6 +4203,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 Fri, Apr 09, 2010 at 04:40:59PM +0200, Jiri Denemark wrote:
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). I don't particularily like introducing a new header file but non of existing header files looked like a good place to stick this macro in. If you think such a place exist, I'll be very happy to adapt the patch.
The second patch changes all API calls introduced since the last 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. But even if my thinking is correct, we don't want to change it before 0.8.0.
In general I think it's a good idea, but instead of adding a new header, I would put it in internals.h for general availability. I agree it's a post-0.8.0 change :-) 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/
participants (2)
-
Daniel Veillard -
Jiri Denemark