9:40 a.m.
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
9:41 a.m.
New subject: [libvirt] [PATCH 1/2] Introduce virCheckFlags for consistent flags checking
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(a)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
9:41 a.m.
New subject: [libvirt] [PATCH 2/2] Use virCheckFlags for APIs added in 0.8.0
---
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
8 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5371
days inactive
5374
days old
3 comments
2 participants
participants (2)
-
Daniel Veillard -
Jiri Denemark