[libvirt] [PATCHv4] libvirt: do not mix internal flags into public API
Laine Stump
laine at laine.org
Mon Jul 18 20:53:59 UTC 2011
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.
More information about the libvir-list
mailing list