[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 3/7] virConnectGetCPUModelNames: implement the remote protocol



On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
> Signed-off-by: Giuseppe Scrivano <gscrivan redhat com>
> ---
>  daemon/remote.c              | 43 +++++++++++++++++++++++++++++++++
>  src/remote/remote_driver.c   | 57 ++++++++++++++++++++++++++++++++++++++++++++
>  src/remote/remote_protocol.x | 20 +++++++++++++++-
>  src/remote_protocol-structs  | 11 +++++++++
>  4 files changed, 130 insertions(+), 1 deletion(-)
> 

>  static int
> +remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                                      virNetServerClientPtr client ATTRIBUTE_UNUSED,
> +                                      virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                                      virNetMessageErrorPtr rerr,
> +                                      remote_connect_get_cpu_model_names_args *args,
> +                                      remote_connect_get_cpu_model_names_ret *ret)
> +{
> +    int len, rv = -1;
> +    char **models;
> +    struct daemonClientPrivate *priv =
> +        virNetServerClientGetPrivateData(client);
> +
> +    if (!priv->conn) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> +        goto cleanup;
> +    }
> +
> +    len = virConnectGetCPUModelNames(priv->conn, args->arch, &models,
> +                                     args->flags);

Needs a tweak to pass NULL on to the driver.

> +    if (len < 0)
> +        goto cleanup;
> +
> +    if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) {

Wrong variable; here, you want to check if we can encode 'len' into ret.

> 
> +        virReportError(VIR_ERR_RPC,
> +                       _("Too many CPU models '%d' for limit '%d'"),
> +                       ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX);
> +        virStringFreeList(models);
> +        goto cleanup;
> +    }
> +
> +    ret->models.models_val = models;
> +    ret->models.models_len = len;

Doesn't work when len is 0 - in that case, models is non-NULL (space to
a lone NULL pointer terminating the empty array), but xdr wants us to
pass NULL, and we must still free models in that case.

> +++ b/src/remote/remote_driver.c
> @@ -5541,6 +5541,62 @@ done:
>  
>  
>  static int
> +remoteConnectGetCPUModelNames(virConnectPtr conn,
> +                              const char *arch,
> +                              char ***models,
> +                              unsigned int flags)
> +{
> +    int rv = -1;
> +    size_t i;
> +    char **retmodels;
> +    remote_connect_get_cpu_model_names_args args;
> +    remote_connect_get_cpu_model_names_ret ret;
> +    struct private_data *priv = conn->privateData;
> +    remoteDriverLock(priv);
> +
> +    memset(&args, 0, sizeof(args));

Pointless, since you end up assigning all members of args below.

> +    memset(&ret, 0, sizeof(ret));
> +
> +    args.arch = (char *) arch;
> +    args.flags = flags;
> +
> +    if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES,
> +             (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args,
> +             (char *) &args,
> +             (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret,
> +             (char *) &ret) < 0)
> +        goto error;
> +
> +    /* Check the length of the returned list carefully. */
> +    if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) {
> +        virReportError(VIR_ERR_RPC, "%s",
> +                       _("remoteConnectGetCPUModelNames: "
> +                         "returned number of CPU models exceeds limit"));

What limit?  Other functions tell us the limit being violated.

> +        goto error;
> +    }
> +
> +    if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0)
> +        goto error;
> +
> +    for (i = 0; i < ret.models.models_len; i++)
> +        retmodels[i] = ret.models.models_val[i];
> +
> +    /* Caller frees MODELS.  */
> +    *models = retmodels;

Needs a tweak to avoid a blind NULL dereference.

> +    rv = ret.models.models_len;
> +
> +done:
> +    remoteDriverUnlock(priv);
> +    return rv;
> +
> +error:
> +    rv = -1;

Dead assignment; rv is already -1 if we get here.

> +    virStringFreeList(ret.models.models_val);

Hmm.  The moment we support a NULL models argument, we have to be
careful that we free models_val always (not just on error); otherwise, a
malicious program on the other end of the rpc pipe could ignore our
request for not needing a result and still cause ret to contain an
allocated list.  I think a cleanup/done pair works for this better than
a done/error pair, modelling after the ListAll methods.

> +    goto done;
> +}
> +
> +
> +static int
>  remoteDomainOpenGraphics(virDomainPtr dom,
>                           unsigned int idx,
>                           int fd,
> @@ -6933,6 +6989,7 @@ static virDriver remote_driver = {
>      .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */
>      .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */
>      .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */
> +    .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */
>  };
>  
>  static virNetworkDriver network_driver = {
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index a1c23da..a1d90ad 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -232,6 +232,9 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64;
>  /* Upper limit on number of job stats */
>  const REMOTE_DOMAIN_JOB_STATS_MAX = 16;
>  
> +/* Upper limit on number of CPU models */
> +const REMOTE_CONNECT_CPU_MODELS_MAX = 8192;
> +

Seems a bit large given that we return just 2 for i686/x86_64 setups,
but no real issue in keeping it that big.

>  /* UUID.  VIR_UUID_BUFLEN definition comes from libvirt.h */
>  typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>  
> @@ -2835,6 +2838,15 @@ struct remote_domain_event_device_removed_msg {
>      remote_nonnull_string devAlias;
>  };
>  
> +struct remote_connect_get_cpu_model_names_args {
> +    remote_nonnull_string arch;
> +    unsigned int flags;
> +};
> +
> +struct remote_connect_get_cpu_model_names_ret {
> +    remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>;

Needs a tweak to support the NULL models.

ACK with this squashed in:

diff --git i/daemon/remote.c w/daemon/remote.c
index d357016..c8afa8f 100644
--- i/daemon/remote.c
+++ w/daemon/remote.c
@@ -5045,7 +5045,7 @@
remoteDispatchConnectGetCPUModelNames(virNetServerPtr server
ATTRIBUTE_UNUSED,

remote_connect_get_cpu_model_names_ret *ret)
 {
     int len, rv = -1;
-    char **models;
+    char **models = NULL;
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);

@@ -5054,27 +5054,36 @@
remoteDispatchConnectGetCPUModelNames(virNetServerPtr server
ATTRIBUTE_UNUSED,
         goto cleanup;
     }

-    len = virConnectGetCPUModelNames(priv->conn, args->arch, &models,
+    len = virConnectGetCPUModelNames(priv->conn, args->arch,
+                                     args->need_results ? &models : NULL,
                                      args->flags);
     if (len < 0)
         goto cleanup;

-    if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) {
+    if (len > REMOTE_CONNECT_CPU_MODELS_MAX) {
         virReportError(VIR_ERR_RPC,
                        _("Too many CPU models '%d' for limit '%d'"),
-                       ret->models.models_len,
REMOTE_CONNECT_CPU_MODELS_MAX);
-        virStringFreeList(models);
+                       len, REMOTE_CONNECT_CPU_MODELS_MAX);
         goto cleanup;
     }

-    ret->models.models_val = models;
-    ret->models.models_len = len;
+    if (len && models) {
+        ret->models.models_val = models;
+        ret->models.models_len = len;
+        models = NULL;
+    } else {
+        ret->models.models_val = NULL;
+        ret->models.models_len = 0;
+    }
+
+    ret->ret = len;

     rv = 0;

 cleanup:
     if (rv < 0)
         virNetMessageSaveError(rerr);
+    virStringFreeList(models);
     return rv;
 }

diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c
index aa3762a..96ccb99 100644
--- i/src/remote/remote_driver.c
+++ w/src/remote/remote_driver.c
@@ -5548,51 +5548,57 @@ remoteConnectGetCPUModelNames(virConnectPtr conn,
 {
     int rv = -1;
     size_t i;
-    char **retmodels;
+    char **retmodels = NULL;
     remote_connect_get_cpu_model_names_args args;
     remote_connect_get_cpu_model_names_ret ret;
+
     struct private_data *priv = conn->privateData;
-    remoteDriverLock(priv);

-    memset(&args, 0, sizeof(args));
-    memset(&ret, 0, sizeof(ret));
+    remoteDriverLock(priv);

     args.arch = (char *) arch;
+    args.need_results = !!models;
     args.flags = flags;

+    memset(&ret, 0, sizeof(ret));
     if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES,
              (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args,
              (char *) &args,
              (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret,
              (char *) &ret) < 0)
-        goto error;
+        goto done;

     /* Check the length of the returned list carefully. */
     if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) {
-        virReportError(VIR_ERR_RPC, "%s",
-                       _("remoteConnectGetCPUModelNames: "
-                         "returned number of CPU models exceeds limit"));
-        goto error;
+        virReportError(VIR_ERR_RPC,
+                       _("Too many model names '%d' for limit '%d'"),
+                       ret.models.models_len,
+                       REMOTE_CONNECT_CPU_MODELS_MAX);
+        goto cleanup;
     }

-    if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0)
-        goto error;
+    if (models) {
+        if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0)
+            goto cleanup;

-    for (i = 0; i < ret.models.models_len; i++)
-        retmodels[i] = ret.models.models_val[i];
+        for (i = 0; i < ret.models.models_len; i++) {
+            retmodels[i] = ret.models.models_val[i];
+            ret.models.models_val[i] = NULL;
+        }
+        *models = retmodels;
+        retmodels = NULL;
+    }

-    /* Caller frees MODELS.  */
-    *models = retmodels;
-    rv = ret.models.models_len;
+    rv = ret.ret;
+
+cleanup:
+    virStringFreeList(retmodels);
+
+    xdr_free((xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret,
(char *) &ret);

 done:
     remoteDriverUnlock(priv);
     return rv;
-
-error:
-    rv = -1;
-    virStringFreeList(ret.models.models_val);
-    goto done;
 }


diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x
index bb8b0ce..8d17bad 100644
--- i/src/remote/remote_protocol.x
+++ w/src/remote/remote_protocol.x
@@ -2840,11 +2840,13 @@ struct remote_domain_event_device_removed_msg {

 struct remote_connect_get_cpu_model_names_args {
     remote_nonnull_string arch;
+    int need_results;
     unsigned int flags;
 };

 struct remote_connect_get_cpu_model_names_ret {
     remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>;
+    int ret;
 };

 /*----- Protocol. -----*/
diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs
index 9148202..98d2d5b 100644
--- i/src/remote_protocol-structs
+++ w/src/remote_protocol-structs
@@ -2318,6 +2318,7 @@ struct remote_domain_event_device_removed_msg {
 };
 struct remote_connect_get_cpu_model_names_args {
         remote_nonnull_string      arch;
+        int                        need_results;
         u_int                      flags;
 };
 struct remote_connect_get_cpu_model_names_ret {
@@ -2325,6 +2326,7 @@ struct remote_connect_get_cpu_model_names_ret {
                 u_int              models_len;
                 remote_nonnull_string * models_val;
         } models;
+        int                        ret;
 };
 enum remote_procedure {
         REMOTE_PROC_CONNECT_OPEN = 1,


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]