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

Re: [libvirt] [PATCH 2/8] Add virDomain{Set, Get}BlockIoTune support to the remote driver



On 11/15/2011 02:02 AM, Lei Li wrote:
> Support Block I/O Throttle setting and query to remote driver.
> 
> Signed-off-by: Lei Li <lilei linux vnet ibm com>
> Signed-off-by: Zhi Yong Wu <wuzhy linux vnet ibm com>
> ---
>  daemon/remote.c              |  109 ++++++++++++++++++++++++++++++++++++++++++
>  src/remote/remote_driver.c   |   96 +++++++++++++++++++++++++++++++++++++
>  src/remote/remote_protocol.x |   27 ++++++++++-
>  src/remote_protocol-structs  |   24 +++++++++
>  4 files changed, 255 insertions(+), 1 deletions(-)
> 

> diff --git a/daemon/remote.c b/daemon/remote.c
> index aa3f768..227d36e 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1886,6 +1886,115 @@ cleanup:
>      return rv;
>  }
>  
> +static int
> +remoteDispatchDomainSetBlockIoThrottle(virNetServerPtr server ATTRIBUTE_UNUSED,

This should be named remoteDispatchDomainSetBlockIoTune, to match the
public API naming.

In fact, by naming it correctly, we can rely on autogen to write this
function for us.

> +
> +static int
> +remoteDispatchDomainGetBlockIoThrottle(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                                       virNetServerClientPtr client ATTRIBUTE_UNUSED,
> +                                       virNetMessagePtr hdr ATTRIBUTE_UNUSED,
> +                                       virNetMessageErrorPtr rerr,
> +                                       remote_domain_get_block_io_throttle_args *args,
> +                                       remote_domain_get_block_io_throttle_ret *ret)

Again, naming should be consistent, but this time we have to provide a
manual version.

> +{
> +    virDomainPtr dom = NULL;
> +    int rv = -1;
> +    int i;
> +    virTypedParameterPtr params;
> +    int nparams = args->nparams;
> +    struct daemonClientPrivate *priv =
> +        virNetServerClientGetPrivateData(client);
> +
> +    if (!priv->conn) {
> +        virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> +        goto cleanup;
> +    }
> +
> +    if (nparams > REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX) {

More naming; the constant should be
REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX.

> +        virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC_N(params, nparams) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
> +        goto cleanup;
> +
> +    if (virDomainGetBlockIoTune(dom, args->disk, params, &nparams, args->flags) < 0)

Given my tweaks in 1/8, we want to be able to handle NULL over the wire;
this means that this type is changed to remote_string, and that
args->disk is now char** instead of char* and requires slightly
different handling.

> +
> +cleanup:
> +    if (rv < 0) {
> +        virNetMessageSaveError(rerr);
> +        if (ret->params.params_val) {
> +            for (i = 0; i < nparams; i++)
> +                VIR_FREE(ret->params.params_val[i].field);
> +            VIR_FREE(ret->params.params_val);
> +        }
> +    }

Memory leak on any string parameters, as well as on params itself.

> +++ b/src/remote/remote_driver.c
> @@ -2178,6 +2178,100 @@ done:
>      return rv;
>  }
>  
> +static int remoteDomainSetBlockIoTune(virDomainPtr domain,

This one can be auto-generated.

> +
> +static int remoteDomainGetBlockIoTune(virDomainPtr domain,
> +                                      const char *disk,
> +                                      virTypedParameterPtr params,
> +                                      int *nparams,
> +                                      unsigned int flags)
> +{
> +    int rv = -1;
> +    remote_domain_get_block_io_throttle_args args;
> +    remote_domain_get_block_io_throttle_ret ret;

Naming of these types.

> +    struct private_data *priv = domain->conn->privateData;
> +
> +    remoteDriverLock(priv);
> +
> +    make_nonnull_domain(&args.dom, domain);
> +    args.disk = (char *)disk;

Type of this argument.

> +    if (remoteDeserializeTypedParameters(ret.params.params_val,
> +                                         ret.params.params_len,
> +                                         REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX,
> +                                         params,
> +                                         nparams) < 0)
> +        goto cleanup;
> +
> +    rv = 0;
> +
> +cleanup:
> +    xdr_free ((xdrproc_t) xdr_remote_domain_get_block_io_throttle_ret,
> +              (char *) &ret);
> +    rv = 0;

Oops - that negated any previously reported error.

> +++ b/src/remote/remote_protocol.x
> @@ -125,6 +125,9 @@ const REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX = 16;
>  /* Upper limit on list of memory parameters. */
>  const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16;
>  
> +/* Upper limit on list of blockio throttle parameters. */
> +const REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX = 16;

As mentioned above, I tweaked this naming.

> +
> +struct remote_domain_get_block_io_throttle_args {
> +    remote_nonnull_domain dom;
> +    remote_nonnull_string disk;

as well as this.

> @@ -2564,7 +2586,10 @@ enum remote_procedure {
>      REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN = 246, /* autogen autogen priority:high */
>      REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, /* autogen autogen priority:high */
>      REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE = 248, /* skipgen skipgen */
> -    REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249 /* skipgen skipgen */
> +    REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249, /* skipgen skipgen */
> +    REMOTE_PROC_DOMAIN_SET_BLOCK_IO_THROTTLE = 250, /* skipgen skipgen */

Missing blank line after a multiple of 10.

> +    REMOTE_PROC_DOMAIN_GET_BLOCK_IO_THROTTLE = 251 /* skipgen skipgen */
> +
>  
>      /*
>       * Notice how the entries are grouped in sets of 10 ?

Here's what I'll be squashing in, when I'm ready to push this:

diff --git i/daemon/remote.c w/daemon/remote.c
index e74c671..2a2c551 100644
--- i/daemon/remote.c
+++ w/daemon/remote.c
@@ -1886,53 +1886,12 @@ cleanup:
 }

 static int
-remoteDispatchDomainSetBlockIoThrottle(virNetServerPtr server
ATTRIBUTE_UNUSED,
-                                       virNetServerClientPtr client
ATTRIBUTE_UNUSED,
-                                       virNetMessagePtr hdr
ATTRIBUTE_UNUSED,
-                                       virNetMessageErrorPtr rerr,
-
remote_domain_set_block_io_throttle_args *args)
-{
-    virDomainPtr dom = NULL;
-    int rv = -1;
-    virTypedParameterPtr params = NULL;
-    int nparams;
-    struct daemonClientPrivate *priv =
-        virNetServerClientGetPrivateData(client);
-
-    if (!priv->conn) {
-        virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
-        goto cleanup;
-    }
-
-    if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
-        goto cleanup;
-
-    if ((params = remoteDeserializeTypedParameters(args->params.params_val,
-                                                   args->params.params_len,
-
REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX,
-                                                   &nparams)) == NULL)
-        goto cleanup;
-
-    rv = virDomainSetBlockIoTune(dom, args->disk, params, nparams,
args->flags);
-
-    if (rv < 0)
-        goto cleanup;
-
-cleanup:
-    if (rv < 0)
-        virNetMessageSaveError(rerr);
-    if (dom)
-        virDomainFree(dom);
-    return rv;
-}
-
-static int
-remoteDispatchDomainGetBlockIoThrottle(virNetServerPtr server
ATTRIBUTE_UNUSED,
-                                       virNetServerClientPtr client
ATTRIBUTE_UNUSED,
-                                       virNetMessagePtr hdr
ATTRIBUTE_UNUSED,
-                                       virNetMessageErrorPtr rerr,
-
remote_domain_get_block_io_throttle_args *args,
-
remote_domain_get_block_io_throttle_ret *ret)
+remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED,
+                                   virNetServerClientPtr client
ATTRIBUTE_UNUSED,
+                                   virNetMessagePtr hdr ATTRIBUTE_UNUSED,
+                                   virNetMessageErrorPtr rerr,
+                                   remote_domain_get_block_io_tune_args
*args,
+                                   remote_domain_get_block_io_tune_ret
*ret)
 {
     virDomainPtr dom = NULL;
     int rv = -1;
@@ -1947,7 +1906,7 @@
remoteDispatchDomainGetBlockIoThrottle(virNetServerPtr server
ATTRIBUTE_UNUSED,
         goto cleanup;
     }

-    if (nparams > REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX) {
+    if (nparams > REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX) {
         virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
         goto cleanup;
     }
@@ -1960,7 +1919,8 @@
remoteDispatchDomainGetBlockIoThrottle(virNetServerPtr server
ATTRIBUTE_UNUSED,
     if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
         goto cleanup;

-    if (virDomainGetBlockIoTune(dom, args->disk, params, &nparams,
args->flags) < 0)
+    if (virDomainGetBlockIoTune(dom, args->disk ? *args->disk : NULL,
+                                params, &nparams, args->flags) < 0)
         goto cleanup;

     /* In this case, we need to send back the number of parameters
@@ -1971,7 +1931,7 @@
remoteDispatchDomainGetBlockIoThrottle(virNetServerPtr server
ATTRIBUTE_UNUSED,
         goto success;
     }

-    /* Serialise the block I/O throttle. */
+    /* Serialise the block I/O tuning parameters. */
     if (remoteSerializeTypedParameters(params, nparams,
                                        &ret->params.params_val,
                                        &ret->params.params_len,
@@ -1982,14 +1942,10 @@ success:
     rv = 0;

 cleanup:
-    if (rv < 0) {
+    if (rv < 0)
         virNetMessageSaveError(rerr);
-        if (ret->params.params_val) {
-            for (i = 0; i < nparams; i++)
-                VIR_FREE(ret->params.params_val[i].field);
-            VIR_FREE(ret->params.params_val);
-        }
-    }
+    virTypedParameterArrayClear(params, nparams);
+    VIR_FREE(params);
     if (dom)
         virDomainFree(dom);
     return rv;
diff --git i/src/libvirt.c w/src/libvirt.c
index a64bc07..074bc22 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -17336,4 +17336,3 @@ error:
     virDispatchError(dom->conn);
     return -1;
 }
-
diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c
index fa2d2c7..c37954e 100644
--- i/src/remote/remote_driver.c
+++ w/src/remote/remote_driver.c
@@ -2178,43 +2178,6 @@ done:
     return rv;
 }

-static int remoteDomainSetBlockIoTune(virDomainPtr domain,
-                                      const char *disk,
-                                      virTypedParameterPtr params,
-                                      int nparams,
-                                      unsigned int flags)
-{
-    int rv = -1;
-    remote_domain_set_block_io_throttle_args args;
-    struct private_data *priv = domain->conn->privateData;
-
-    remoteDriverLock(priv);
-
-    memset(&args, 0, sizeof(args));
-
-    make_nonnull_domain(&args.dom, domain);
-    args.disk = (char *)disk;
-    args.flags = flags;
-
-    if (remoteSerializeTypedParameters(params, nparams,
&args.params.params_val, &args.params.params_len) < 0) {
-
xdr_free((xdrproc_t)xdr_remote_domain_set_block_io_throttle_args, (char
*)&args);
-        goto done;
-    }
-
-    if (call(domain->conn, priv, 0,
REMOTE_PROC_DOMAIN_SET_BLOCK_IO_THROTTLE,
-             (xdrproc_t) xdr_remote_domain_set_block_io_throttle_args,
-               (char *) &args,
-             (xdrproc_t) xdr_void,
-               (char *) NULL) == -1) {
-        goto done;
-    }
-    rv = 0;
-
-done:
-    remoteDriverUnlock(priv);
-    return rv;
-}
-
 static int remoteDomainGetBlockIoTune(virDomainPtr domain,
                                       const char *disk,
                                       virTypedParameterPtr params,
@@ -2222,24 +2185,24 @@ static int
remoteDomainGetBlockIoTune(virDomainPtr domain,
                                       unsigned int flags)
 {
     int rv = -1;
-    remote_domain_get_block_io_throttle_args args;
-    remote_domain_get_block_io_throttle_ret ret;
+    remote_domain_get_block_io_tune_args args;
+    remote_domain_get_block_io_tune_ret ret;
     struct private_data *priv = domain->conn->privateData;

     remoteDriverLock(priv);

     make_nonnull_domain(&args.dom, domain);
-    args.disk = (char *)disk;
+    args.disk = disk ? (char **)&disk : NULL;
     args.nparams = *nparams;
     args.flags = flags;

     memset(&ret, 0, sizeof(ret));


-    if (call(domain->conn, priv, 0,
REMOTE_PROC_DOMAIN_GET_BLOCK_IO_THROTTLE,
-             (xdrproc_t) xdr_remote_domain_get_block_io_throttle_args,
+    if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE,
+             (xdrproc_t) xdr_remote_domain_get_block_io_tune_args,
                (char *) &args,
-             (xdrproc_t) xdr_remote_domain_get_block_io_throttle_ret,
+             (xdrproc_t) xdr_remote_domain_get_block_io_tune_ret,
                (char *) &ret) == -1) {
         goto done;
     }
@@ -2263,10 +2226,8 @@ static int
remoteDomainGetBlockIoTune(virDomainPtr domain,
     rv = 0;

 cleanup:
-    xdr_free ((xdrproc_t) xdr_remote_domain_get_block_io_throttle_ret,
+    xdr_free ((xdrproc_t) xdr_remote_domain_get_block_io_tune_ret,
               (char *) &ret);
-    rv = 0;
-
 done:
     remoteDriverUnlock(priv);
     return rv;
diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x
index 9ab3314..106a982 100644
--- i/src/remote/remote_protocol.x
+++ w/src/remote/remote_protocol.x
@@ -125,8 +125,8 @@ const REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX = 16;
 /* Upper limit on list of memory parameters. */
 const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16;

-/* Upper limit on list of blockio throttle parameters. */
-const REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX = 16;
+/* Upper limit on list of blockio tuning parameters. */
+const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 16;

 /* Upper limit on list of node cpu stats. */
 const REMOTE_NODE_CPU_STATS_MAX = 16;
@@ -1078,22 +1078,22 @@ struct remote_domain_block_pull_args {
     unsigned int flags;
 };

-struct remote_domain_set_block_io_throttle_args {
+struct remote_domain_set_block_io_tune_args {
     remote_nonnull_domain dom;
     remote_nonnull_string disk;
-    remote_typed_param params<REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX>;
+    remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>;
     unsigned int flags;
 };

-struct remote_domain_get_block_io_throttle_args {
+struct remote_domain_get_block_io_tune_args {
     remote_nonnull_domain dom;
-    remote_nonnull_string disk;
+    remote_string disk;
     int nparams;
     unsigned int flags;
 };

-struct remote_domain_get_block_io_throttle_ret {
-    remote_typed_param params<REMOTE_DOMAIN_BLKIOTHROTTLE_PARAMETERS_MAX>;
+struct remote_domain_get_block_io_tune_ret {
+    remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>;
     int nparams;
 };

@@ -2587,9 +2587,9 @@ enum remote_procedure {
     REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, /* autogen
autogen priority:high */
     REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE = 248, /* skipgen skipgen */
     REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249, /* skipgen skipgen */
-    REMOTE_PROC_DOMAIN_SET_BLOCK_IO_THROTTLE = 250, /* skipgen skipgen */
-    REMOTE_PROC_DOMAIN_GET_BLOCK_IO_THROTTLE = 251 /* skipgen skipgen */
+    REMOTE_PROC_DOMAIN_SET_BLOCK_IO_TUNE = 250, /* autogen autogen */

+    REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE = 251 /* skipgen skipgen */

     /*
      * Notice how the entries are grouped in sets of 10 ?
diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs
index eabf5e5..853eac4 100644
--- i/src/remote_protocol-structs
+++ w/src/remote_protocol-structs
@@ -759,22 +759,22 @@ struct remote_domain_block_pull_args {
         uint64_t                   bandwidth;
         u_int                      flags;
 };
-struct remote_domain_set_block_io_throttle_args {
+struct remote_domain_set_block_io_tune_args {
         remote_nonnull_domain      dom;
-        remote_nonnull_string disk;
+        remote_nonnull_string      disk;
         struct {
                 u_int              params_len;
                 remote_typed_param * params_val;
         } params;
         u_int                      flags;
 };
-struct remote_domain_get_block_io_throttle_args {
+struct remote_domain_get_block_io_tune_args {
         remote_nonnull_domain      dom;
-        remote_nonnull_string disk;
+        remote_string              disk;
         int                        nparams;
         u_int                      flags;
 };
-struct remote_domain_get_block_io_throttle_ret {
+struct remote_domain_get_block_io_tune_ret {
         struct {
                 u_int              params_len;
                 remote_typed_param * params_val;
@@ -2029,6 +2029,6 @@ enum remote_procedure {
         REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247,
         REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE = 248,
         REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249,
-        REMOTE_PROC_DOMAIN_SET_BLOCK_IO_THROTTLE = 250,
-        REMOTE_PROC_DOMAIN_GET_BLOCK_IO_THROTTLE = 251,
+        REMOTE_PROC_DOMAIN_SET_BLOCK_IO_TUNE = 250,
+        REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE = 251,
 };


-- 
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]