On 07/15/2011 07:12 PM, Eric Blake wrote:

There were two API in driver.c that were silently masking flags bits prior to calling out to the drivers, and several others that were explicitly masking flags bits. 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. Additionally, virDomainMemoryStats passed a flags argument over RPC, but not to the driver.

* src/driver.h (VIR_DOMAIN_XML_FLAGS_MASK) (VIR_SECRET_GET_VALUE_FLAGS_MASK): Delete. (virDrvSecretGetValue): Separate out internal flags. (virDrvDomainMemoryStats): Provide missing flags argument. * src/driver.c (verify): Drop unused check. * src/conf/domain_conf.h (virDomainObjParseFile): Delete declaration. (virDomainXMLInternalFlags): Move... * src/conf/domain_conf.c: ...here. Delete redundant include. (virDomainObjParseFile): Make static. * src/libvirt.c (virDomainGetXMLDesc, virSecretGetValue): Update clients. (virDomainMemoryPeek, virInterfaceGetXMLDesc) (virDomainMemoryStats, virDomainBlockPeek, virNetworkGetXMLDesc) (virStoragePoolGetXMLDesc, virStorageVolGetXMLDesc) (virNodeNumOfDevices, virNodeListDevices, virNWFilterGetXMLDesc): Don't mask unknown flags. * src/interface/netcf_driver.c (interfaceGetXMLDesc): Reject unknown flags. * src/secret/secret_driver.c (secretGetValue): Update clients. * src/remote/remote_driver.c (remoteSecretGetValue) (remoteDomainMemoryStats): Likewise. * src/qemu/qemu_process.c (qemuProcessGetVolumeQcowPassphrase): Likewise. * src/qemu/qemu_driver.c (qemudDomainMemoryStats): Likewise. * daemon/remote.c (remoteDispatchDomainMemoryStats): Likewise. ---

v4: fix even more public libvirt API flag masking v3 at https://www.redhat.com/archives/libvir-list/2011-July/msg00894.html

daemon/remote.c | 2 +- src/conf/domain_conf.c | 16 +++++++++---- src/conf/domain_conf.h | 10 -------- src/driver.c | 4 --- src/driver.h | 16 +++++-------- src/interface/netcf_driver.c | 2 + src/libvirt.c | 51 +++++------------------------------------ src/qemu/qemu_driver.c | 5 +++- src/qemu/qemu_process.c | 2 +- src/remote/remote_driver.c | 13 ++++++++-- src/secret/secret_driver.c | 7 ++++- 11 files changed, 47 insertions(+), 81 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index e93a0dc..daad39d 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -810,7 +810,7 @@ remoteDispatchDomainMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; }

- nr_stats = virDomainMemoryStats(dom, stats, args->maxStats, 0); + nr_stats = virDomainMemoryStats(dom, stats, args->maxStats, args->flags); if (nr_stats< 0) goto cleanup;

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8c3e44e..3c3ab39 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 */

I was wondering why you didn't make this (1<<0), but you pointed out on IRC that you're still using the same flag arg for both public and internal flags in domain_conf.c (just checking for conflict at compile time with verify()).

+} 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; 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..9d0d3de 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, @@ -331,7 +331,8 @@ typedef int (*virDrvDomainMemoryStats) (virDomainPtr domain, struct _virDomainMemoryStat *stats, - unsigned int nr_stats); + unsigned int nr_stats, + unsigned int flags);

typedef int (*virDrvDomainBlockPeek) @@ -1229,16 +1230,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 +1258,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/interface/netcf_driver.c b/src/interface/netcf_driver.c index 855b5a3..2f322b4 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/netcf_driver.c @@ -344,6 +344,8 @@ static char *interfaceGetXMLDesc(virInterfacePtr ifinfo, virInterfaceDefPtr ifacedef = NULL; char *ret = NULL;

+ virCheckFlags(VIR_INTERFACE_XML_INACTIVE, NULL); +

If I understand correctly, this is here to put the burden on the driver to check/reject flags, rather than letting the client do it.

interfaceDriverLock(driver);

iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo); diff --git a/src/libvirt.c b/src/libvirt.c index 7c68bd8..4de718d 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); @@ -6031,11 +6029,6 @@ int virDomainMemoryStats (virDomainPtr dom, virDomainMemoryStatPtr stats, virDispatchError(NULL); return -1; } - if (flags != 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("flags must be zero")); - goto error; - }

if (!stats || nr_stats == 0) return 0; @@ -6045,7 +6038,8 @@ int virDomainMemoryStats (virDomainPtr dom, virDomainMemoryStatPtr stats,

conn = dom->conn; if (conn->driver->domainMemoryStats) { - nr_stats_ret = conn->driver->domainMemoryStats (dom, stats, nr_stats); + nr_stats_ret = conn->driver->domainMemoryStats (dom, stats, nr_stats, + flags); if (nr_stats_ret == -1) goto error; return nr_stats_ret; @@ -6130,12 +6124,6 @@ virDomainBlockPeek (virDomainPtr dom, goto error; }

- if (flags != 0) { - virLibDomainError(VIR_ERR_INVALID_ARG, - _("flags must be zero")); - goto error; - } - /* Allow size == 0 as an access test. */ if (size> 0&& !buffer) { virLibDomainError(VIR_ERR_INVALID_ARG, @@ -6239,9 +6227,10 @@ virDomainMemoryPeek (virDomainPtr dom, * because of incompatible licensing. */

- if (flags != VIR_MEMORY_VIRTUAL&& flags != VIR_MEMORY_PHYSICAL) { + /* Exactly one of these two flags must be set. */ + if (!(flags& VIR_MEMORY_VIRTUAL) == !(flags& VIR_MEMORY_PHYSICAL)) { virLibDomainError(VIR_ERR_INVALID_ARG, - _("flags parameter must be VIR_MEMORY_VIRTUAL or VIR_MEMORY_PHYSICAL")); + _("flags parameter must include VIR_MEMORY_VIRTUAL or VIR_MEMORY_PHYSICAL")); goto error; }

@@ -8465,10 +8454,6 @@ virNetworkGetXMLDesc(virNetworkPtr network, unsigned int flags) virDispatchError(NULL); return NULL; } - if (flags != 0) { - virLibNetworkError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - }

conn = network->conn;

@@ -8978,10 +8963,6 @@ virInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags) virDispatchError(NULL); return NULL; } - if ((flags& ~VIR_INTERFACE_XML_INACTIVE) != 0) { - virLibInterfaceError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - }

conn = iface->conn;

@@ -10451,10 +10432,6 @@ virStoragePoolGetXMLDesc(virStoragePoolPtr pool, virDispatchError(NULL); return NULL; } - if (flags != 0) { - virLibStoragePoolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - }

conn = pool->conn;

@@ -11349,10 +11326,6 @@ virStorageVolGetXMLDesc(virStorageVolPtr vol, virDispatchError(NULL); return NULL; } - if (flags != 0) { - virLibStorageVolError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - }

conn = vol->conn;

@@ -11441,10 +11414,6 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) virDispatchError(NULL); return -1; } - if (flags != 0) { - virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - }

if (conn->deviceMonitor&& conn->deviceMonitor->numOfDevices) { int ret; @@ -11493,7 +11462,7 @@ virNodeListDevices(virConnectPtr conn, virDispatchError(NULL); return -1; } - if ((flags != 0) || (names == NULL) || (maxnames< 0)) { + if ((names == NULL) || (maxnames< 0)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } @@ -12699,12 +12668,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);

So the getValue() callback has an extra internalFlags arg, but that is only non-0 if get Value is called from the driver side (e.g. from qemu), and never sent over the wire. A bit odd, but I can live with it (or you can add the v5 interdiff - either way is fine with me).

if (ret == NULL) goto error; return ret; @@ -14233,10 +14200,6 @@ virNWFilterGetXMLDesc(virNWFilterPtr nwfilter, unsigned int flags) virDispatchError(NULL); return NULL; } - if (flags != 0) { - virLibNWFilterError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - }

conn = nwfilter->conn;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8d146aa..1a3fbfb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6155,12 +6155,15 @@ qemudDomainInterfaceStats (virDomainPtr dom, static int qemudDomainMemoryStats (virDomainPtr dom, struct _virDomainMemoryStat *stats, - unsigned int nr_stats) + unsigned int nr_stats, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; unsigned int ret = -1;

+ virCheckFlags(0, -1); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); 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);

Here's where the internalFlags arg is non-zero, and it's all on the libvirtd/driver side.

virUnrefSecret(secret); if (data == NULL) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 2d5dc15..c2f8bbd 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1850,7 +1850,8 @@ done: static int remoteDomainMemoryStats (virDomainPtr domain, struct _virDomainMemoryStat *stats, - unsigned int nr_stats) + unsigned int nr_stats, + unsigned int flags) { int rv = -1; remote_domain_memory_stats_args args; @@ -1868,7 +1869,7 @@ remoteDomainMemoryStats (virDomainPtr domain, goto done; } args.maxStats = nr_stats; - args.flags = 0; + args.flags = flags; memset (&ret, 0, sizeof ret);

if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_MEMORY_STATS, @@ -3173,7 +3174,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 +3183,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"));

ACK, either with or without the v5 interdiff added. Having a separate callback seems cleaner, but may have other implications I'm not aware of.