From 33ba6e688149e663ca4e2f41f130e7d4835c3c80 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Wed, 13 Jul 2011 15:31:56 -0600
Subject: [PATCH] libvirt: do not mix internal flags into public API

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.
---
 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 e93a0dcba0..daad39d3a5 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 8c3e44ed88..3c3ab39699 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 */
+} 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 172d3c2878..7a3d72b9e8 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 579c2b3485..5034277838 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 70ea4c235f..9d0d3de9c4 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 855b5a3ed7..2f322b4db9 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);
+
     interfaceDriverLock(driver);
 
     iface = interfaceDriverGetNetcfIF(driver->netcf, ifinfo);
diff --git a/src/libvirt.c b/src/libvirt.c
index 001c38dd75..39e2041e40 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);
@@ -6040,11 +6038,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;
@@ -6054,7 +6047,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;
@@ -6139,12 +6133,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,
@@ -6248,9 +6236,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;
     }
 
@@ -8474,10 +8463,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;
 
@@ -8987,10 +8972,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;
 
@@ -10460,10 +10441,6 @@ virStoragePoolGetXMLDesc(virStoragePoolPtr pool,
         virDispatchError(NULL);
         return NULL;
     }
-    if (flags != 0) {
-        virLibStoragePoolError(VIR_ERR_INVALID_ARG, __FUNCTION__);
-        goto error;
-    }
 
     conn = pool->conn;
 
@@ -11358,10 +11335,6 @@ virStorageVolGetXMLDesc(virStorageVolPtr vol,
         virDispatchError(NULL);
         return NULL;
     }
-    if (flags != 0) {
-        virLibStorageVolError(VIR_ERR_INVALID_ARG, __FUNCTION__);
-        goto error;
-    }
 
     conn = vol->conn;
 
@@ -11450,10 +11423,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;
@@ -11502,7 +11471,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;
     }
@@ -12708,12 +12677,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);
         if (ret == NULL)
             goto error;
         return ret;
@@ -14242,10 +14209,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 8d54e58a9b..8870e33eeb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6166,12 +6166,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 d0085e0eef..448b06e061 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);
     virUnrefSecret(secret);
     if (data == NULL)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 2d5dc15171..c2f8bbde06 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 c45ba51147..02cdbb9122 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"));