On 01/27/2012 11:21 PM, KAMEZAWA Hiroyuki wrote:
> remote protocol driver for virDomainGetCPUStats()
>
> Note:
> Unlike other users of virTypedParameter with RPC, this interface
> can return zero-filled entries because the interface assumes
> 2 dimentional array. Then, the function has its own serialize/deserialize
s/dimentional/dimensional/
> routine.
>
> daemon/remote.c | 146 +++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_driver.c | 117 ++++++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 22 ++++++
> src/remote_protocol-structs | 15 ++++
> ---
> daemon/remote.c | 147 ++++++++++++++++++++++++++++++++++++++++++
> src/remote/remote_driver.c | 117 +++++++++++++++++++++++++++++++++
> src/remote/remote_protocol.x | 22 ++++++-
> src/remote_protocol-structs | 15 +++
Odd that the diffstat listed twice.
I'm going to reorder my review, to start with the .x file.
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 0f354bb..205aeeb 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -208,6 +208,12 @@ const REMOTE_DOMAIN_SEND_KEY_MAX = 16;
> */
> const REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX = 16;
>
> +/*
> + * Upper limit on list of (real) cpu parameters
> + * nparams(<=16) * ncpus(<=128) <= 2048.
> + */
> +const REMOTE_DOMAIN_GET_CPU_STATS_MAX = 2048;
We should enforce the two limits independently, which means we also need
two smaller constants (it's a shame that rpcgen doesn't allow products
when computing a constant, but we can document how we arrive at various
numbers). We can reuse VIR_NODE_CPU_STATS_MAX for one of the two limits.
> +
> /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
> typedef opaque remote_uuid[VIR_UUID_BUFLEN];
>
> @@ -1149,6 +1155,19 @@ struct remote_domain_get_block_io_tune_ret {
> int nparams;
> };
>
> +struct remote_domain_get_cpu_stats_args {
> + remote_nonnull_domain dom;
> + unsigned int nparams;
> + int start_cpu;
> + unsigned int ncpus;
> + unsigned int flags;
> +};
> +
> +struct remote_domain_get_cpu_stats_ret {
> + remote_typed_param params<REMOTE_DOMAIN_GET_CPU_STATS_MAX>;
> + int nparams;
> +};
Looks good.
> +
> /* Network calls: */
>
> struct remote_num_of_networks_ret {
> @@ -2667,7 +2686,8 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_SET_INTERFACE_PARAMETERS = 256, /* autogen
autogen */
> REMOTE_PROC_DOMAIN_GET_INTERFACE_PARAMETERS = 257, /* skipgen
skipgen */
> REMOTE_PROC_DOMAIN_SHUTDOWN_FLAGS = 258, /* autogen autogen */
> - REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259 /* autogen autogen */
> + REMOTE_PROC_STORAGE_VOL_WIPE_PATTERN = 259, /* autogen autogen */
> + REMOTE_PROC_DOMAIN_GET_CPU_STATS = 260 /* skipgen skipgen */
Trivial conflict resolution.
> +static int
> +remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client ATTRIBUTE_UNUSED,
> + virNetMessagePtr hdr ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr,
> + remote_domain_get_cpu_stats_args *args,
> + remote_domain_get_cpu_stats_ret *ret)
> +{
> + virDomainPtr dom = NULL;
> + struct daemonClientPrivate *priv;
> + virTypedParameterPtr params = NULL;
> + remote_typed_param *info = NULL;
> + int cpu, idx, src, dest;
> + int rv = -1;
> + unsigned int return_size = 0;
> + int percpu_len = 0;
> +
> + priv = virNetServerClientGetPrivateData(client);
> + if (!priv->conn) {
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> + goto cleanup;
> + }
> +
> + /*
> + * nparams * ncpus should be smaller than
> + * REMOTE_DOMAIN_GET_CPU_STATS_MAX but nparams
> + * and ncpus are limited by API. check it.
> + */
> + if (args->nparams > 16) {
> + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
> + goto cleanup;
> + }
> + if (args->ncpus > 128) {
Use the constants from the .x file, rather than open-coding things.
Oops, I just realized that I mentioned that libvirt.c should not have a
length check, but then I forgot to delete it. Meanwhile, even if
libvirt.c does not have a length check, it still needs to have a
multiplication overflow check.
> + /*
> + * Here, we return values based on the real param size returned by
> + * a driver rather than the passed one. RPC Client stub should decode
> + * it to fit callar's expectation.
s/callar/caller/
> + *
> + * If cpu ID is sparse, The whole array for some cpu IDs will be
> + * zero-cleared. This doesn't meet to remoteSerializeTypedParameters()
> + * and we do serialization by ourselves.
We can still use remoteSerializeTypedParameters; we just have to teach
it to skip zero entries like it already can skip string entries.
Oh, I just realized that to support string entries, we have to support a
flags of VIR_TYPED_PARAM_STRING_OKAY.
> +success:
> + rv = 0;
> + ret->params.params_len = return_size;
> + ret->params.params_val = info;
> + ret->nparams = percpu_len;
> +cleanup:
> + if (rv < 0) {
> + virNetMessageSaveError(rerr);
> + if (info) {
Cleanup is a lot simpler if we let the existing serialization skip zero
entries.
> +++ b/src/remote/remote_driver.c
> @@ -2305,6 +2305,122 @@ done:
> return rv;
> }
>
> +static int remoteDomainGetCPUStats(virDomainPtr domain,
> + virTypedParameterPtr params,
> + unsigned int nparams,
> + int start_cpu,
> + unsigned int ncpus,
> + unsigned int flags)
> +{
> + struct private_data *priv = domain->conn->privateData;
> + remote_domain_get_cpu_stats_args args;
> + remote_domain_get_cpu_stats_ret ret;
> + remote_typed_param *info;
> + int rv = -1;
> + int cpu, idx, src, dest;
> +
> + remoteDriverLock(priv);
> +
> + make_nonnull_domain(&args.dom, domain);
> + args.nparams = nparams;
> + args.start_cpu = start_cpu;
> + args.ncpus = ncpus;
> + args.flags = flags;
> +
> + memset(&ret, 0, sizeof(ret));
This side needs to validate incoming parameters before sending over the
wire. Hmm, remoteNodeGetCPUStats needs to do likewise.
> +
> + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_CPU_STATS,
> + (xdrproc_t) xdr_remote_domain_get_cpu_stats_args,
> + (char *) &args,
> + (xdrproc_t) xdr_remote_domain_get_cpu_stats_ret,
> + (char *) &ret) == -1)
> + goto done;
> +
> + if (nparams == 0) {
> + rv = ret.nparams;
> + goto cleanup;
> + }
> + /*
> + * the returned arrray's size is not same to nparams * ncpus. And
> + * if cpu ID is not contiguous, all-zero entries can be found.
> + */
> + memset(params, 0, sizeof(virTypedParameter) * nparams * ncpus);
I prefer sizeof(expr) over sizeof(type), in case expr ever changes types.
> +
> + /* Here, ret.nparams is always smaller than nparams */
> + info = ret.params.params_val;
> +
> + for (cpu = 0; cpu < ncpus; ++cpu) {
> + for (idx = 0; idx < ret.nparams; ++idx) {
> + src = cpu * ret.nparams + idx;
> + dest = cpu * nparams + idx;
> +
> + if (info[src].value.type == 0) /* skip zeroed ones */
> + continue;
Again, open coding things is not nice. If we write the server to never
send zero entries (which is a good thing - less data over the wire),
then the client needs to re-create zero entries by calling
deserialization in a loop - deserialize ret.nparams entries at every
nparams slot.
> +
> + rv = ret.nparams;
> +cleanup:
> + if (rv < 0) {
> + int max = nparams * ncpus;
> + int i;
> +
> + for (i = 0; i < max; i++) {
> + if (params[i].type == VIR_TYPED_PARAM_STRING)
> + VIR_FREE(params[i].value.s);
Potential free of bogus memory if anything can reach cleanup with rv < 0
but before we sanitized the contents of params.
Here's what I'm squashing in:
diff --git i/daemon/remote.c w/daemon/remote.c
index 15857a8..cb8423a 100644
--- i/daemon/remote.c
+++ w/daemon/remote.c
@@ -704,8 +704,11 @@ remoteSerializeTypedParameters(virTypedParameterPtr
params,
}
for (i = 0, j = 0; i < nparams; ++i) {
- if (!(flags & VIR_TYPED_PARAM_STRING_OKAY) &&
- params[i].type == VIR_TYPED_PARAM_STRING) {
+ /* virDomainGetCPUStats can return a sparse array; also, we
+ * can't pass back strings to older clients. */
+ if (!params[i].type ||
+ (!(flags & VIR_TYPED_PARAM_STRING_OKAY) &&
+ params[i].type == VIR_TYPED_PARAM_STRING)) {
--*ret_params_len;
continue;
}
@@ -3534,10 +3537,7 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr
server ATTRIBUTE_UNUSED,
virDomainPtr dom = NULL;
struct daemonClientPrivate *priv;
virTypedParameterPtr params = NULL;
- remote_typed_param *info = NULL;
- int cpu, idx, src, dest;
int rv = -1;
- unsigned int return_size = 0;
int percpu_len = 0;
priv = virNetServerClientGetPrivateData(client);
@@ -3546,128 +3546,53 @@ remoteDispatchDomainGetCPUStats(virNetServerPtr
server ATTRIBUTE_UNUSED,
goto cleanup;
}
- /*
- * nparams * ncpus should be smaller than
- * REMOTE_DOMAIN_GET_CPU_STATS_MAX but nparams
- * and ncpus are limited by API. check it.
- */
- if (args->nparams > 16) {
+ if (args->nparams > REMOTE_NODE_CPU_STATS_MAX) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
goto cleanup;
}
- if (args->ncpus > 128) {
+ if (args->ncpus > REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX) {
virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("ncpus too large"));
goto cleanup;
}
- if (args->nparams > 0) {
- if (VIR_ALLOC_N(params, args->ncpus * args->nparams) < 0)
- goto no_memory;
+ if (args->nparams > 0 &&
+ VIR_ALLOC_N(params, args->ncpus * args->nparams) < 0) {
+ virReportOOMError();
+ goto cleanup;
}
if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
goto cleanup;
percpu_len = virDomainGetCPUStats(dom, params, args->nparams,
- args->start_cpu, args->ncpus, args->flags);
+ args->start_cpu, args->ncpus,
+ args->flags);
if (percpu_len < 0)
goto cleanup;
- /* If nparams == 0, the function returns a sigle value */
+ /* If nparams == 0, the function returns a single value */
if (args->nparams == 0)
goto success;
- /*
- * Here, we return values based on the real param size returned by
- * a driver rather than the passed one. RPC Client stub should decode
- * it to fit callar's expectation.
- *
- * If cpu ID is sparse, The whole array for some cpu IDs will be
- * zero-cleared. This doesn't meet to remoteSerializeTypedParameters()
- * and we do serialization by ourselves.
- */
- return_size = percpu_len * args->ncpus;
- if (VIR_ALLOC_N(info, return_size) < 0)
- goto no_memory;
+ if (remoteSerializeTypedParameters(params, args->nparams * args->ncpus,
+ &ret->params.params_val,
+ &ret->params.params_len,
+ args->flags) < 0)
+ goto cleanup;
+
+ percpu_len = ret->params.params_len / args->ncpus;
- for (cpu = 0; cpu < args->ncpus; ++cpu) {
- for (idx = 0; idx < percpu_len; ++idx) {
- src = cpu * args->nparams + idx;
- dest = cpu * percpu_len + idx;
-
- /* If CPU ID is discontiguous, this can happen */
- if (params[src].type == 0)
- continue;
-
- info[dest].field = strdup(params[src].field);
- if (info[dest].field == NULL)
- goto no_memory;
-
- info[dest].value.type = params[src].type;
-
- switch (params[src].type) {
- case VIR_TYPED_PARAM_INT:
- info[dest].value.remote_typed_param_value_u.i =
- params[src].value.i;
- break;
- case VIR_TYPED_PARAM_UINT:
- info[dest].value.remote_typed_param_value_u.ui =
- params[src].value.ui;
- break;
- case VIR_TYPED_PARAM_LLONG:
- info[dest].value.remote_typed_param_value_u.l =
- params[src].value.l;
- break;
- case VIR_TYPED_PARAM_ULLONG:
- info[dest].value.remote_typed_param_value_u.ul =
- params[src].value.ul;
- break;
- case VIR_TYPED_PARAM_DOUBLE:
- info[dest].value.remote_typed_param_value_u.d =
- params[src].value.d;
- break;
- case VIR_TYPED_PARAM_BOOLEAN:
- info[dest].value.remote_typed_param_value_u.b =
- params[src].value.b;
- break;
- case VIR_TYPED_PARAM_STRING:
- info[dest].value.remote_typed_param_value_u.s =
- strdup(params[src].value.s);
- if (info[dest].value.remote_typed_param_value_u.s == NULL)
- goto no_memory;
- break;
- default:
- virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"),
- params[src].type);
- goto cleanup;
- }
- }
- }
success:
rv = 0;
- ret->params.params_len = return_size;
- ret->params.params_val = info;
ret->nparams = percpu_len;
+
cleanup:
- if (rv < 0) {
+ if (rv < 0)
virNetMessageSaveError(rerr);
- if (info) {
- int i;
- for (i = 0; i < return_size; i++) {
- VIR_FREE(info[i].field);
- if (info[i].value.type == VIR_TYPED_PARAM_STRING)
- VIR_FREE(info[i].value.remote_typed_param_value_u.s);
- }
- VIR_FREE(info);
- }
- }
virTypedParameterArrayClear(params, args->ncpus * args->nparams);
VIR_FREE(params);
if (dom)
virDomainFree(dom);
return rv;
-no_memory:
- virReportOOMError();
- goto cleanup;
}
/*----- Helpers. -----*/
diff --git i/src/libvirt.c w/src/libvirt.c
index 7060f5a..e702a34 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -18162,7 +18162,7 @@ error:
* @nparams: number of parameters per cpu
* @start_cpu: which cpu to start with, or -1 for summary
* @ncpus: how many cpus to query
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: bitwise-OR of virTypedParameterFlags
*
* Get statistics relating to CPU usage attributable to a single
* domain (in contrast to the statistics returned by
@@ -18251,20 +18251,19 @@ int virDomainGetCPUStats(virDomainPtr domain,
* if start_cpu is -1, ncpus must be 1
* params == NULL must match nparams == 0
* ncpus must be non-zero unless params == NULL
+ * nparams * ncpus must not overflow (RPC may restrict it even more)
*/
if (start_cpu < -1 ||
(start_cpu == -1 && ncpus != 1) ||
((params == NULL) != (nparams == 0)) ||
- (ncpus == 0 && params != NULL)) {
- virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
- goto error;
- }
-
- /* remote protocol doesn't welcome big args in one shot */
- if ((nparams > 16) || (ncpus > 128)) {
+ (ncpus == 0 && params != NULL) ||
+ ncpus < UINT_MAX / nparams) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
if (conn->driver->domainGetCPUStats) {
int ret;
diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c
index a3ca63e..2312cf9 100644
--- i/src/remote/remote_driver.c
+++ w/src/remote/remote_driver.c
@@ -2,7 +2,7 @@
* remote_driver.c: driver to provide access to libvirtd running
* on a remote machine
*
- * Copyright (C) 2007-2011 Red Hat, Inc.
+ * Copyright (C) 2007-2012 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -2315,12 +2315,24 @@ static int remoteDomainGetCPUStats(virDomainPtr
domain,
struct private_data *priv = domain->conn->privateData;
remote_domain_get_cpu_stats_args args;
remote_domain_get_cpu_stats_ret ret;
- remote_typed_param *info;
int rv = -1;
- int cpu, idx, src, dest;
+ int cpu;
remoteDriverLock(priv);
+ if (nparams > REMOTE_NODE_CPU_STATS_MAX) {
+ remoteError(VIR_ERR_RPC,
+ _("nparams count exceeds maximum: %u > %u"),
+ nparams, REMOTE_NODE_CPU_STATS_MAX);
+ goto done;
+ }
+ if (ncpus > REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX) {
+ remoteError(VIR_ERR_RPC,
+ _("ncpus count exceeds maximum: %u > %u"),
+ ncpus, REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX);
+ goto done;
+ }
+
make_nonnull_domain(&args.dom, domain);
args.nparams = nparams;
args.start_cpu = start_cpu;
@@ -2336,67 +2348,38 @@ static int remoteDomainGetCPUStats(virDomainPtr
domain,
(char *) &ret) == -1)
goto done;
+ /* Check the length of the returned list carefully. */
+ if (ret.params.params_len > nparams * ncpus ||
+ (ret.params.params_len &&
+ ret.nparams * ncpus != ret.params.params_len)) {
+ remoteError(VIR_ERR_RPC, "%s",
+ _("remoteDomainGetCPUStats: "
+ "returned number of stats exceeds limit"));
+ memset(params, 0, sizeof(*params) * nparams * ncpus);
+ goto cleanup;
+ }
+
+ /* Handle the case when the caller does not know the number of stats
+ * and is asking for the number of stats supported
+ */
if (nparams == 0) {
rv = ret.nparams;
goto cleanup;
}
- /*
- * the returned arrray's size is not same to nparams * ncpus. And
- * if cpu ID is not contiguous, all-zero entries can be found.
+
+ /* The remote side did not send back any zero entries, so we have
+ * to expand things back into a possibly sparse array.
*/
- memset(params, 0, sizeof(virTypedParameter) * nparams * ncpus);
-
- /* Here, ret.nparams is always smaller than nparams */
- info = ret.params.params_val;
-
- for (cpu = 0; cpu < ncpus; ++cpu) {
- for (idx = 0; idx < ret.nparams; ++idx) {
- src = cpu * ret.nparams + idx;
- dest = cpu * nparams + idx;
-
- if (info[src].value.type == 0) /* skip zeroed ones */
- continue;
-
- params[dest].type = info[src].value.type;
- strcpy(params[dest].field, info[src].field);
-
- switch (params[dest].type) {
- case VIR_TYPED_PARAM_INT:
- params[dest].value.i =
- info[src].value.remote_typed_param_value_u.i;
- break;
- case VIR_TYPED_PARAM_UINT:
- params[dest].value.ui =
- info[src].value.remote_typed_param_value_u.ui;
- break;
- case VIR_TYPED_PARAM_LLONG:
- params[dest].value.l =
- info[src].value.remote_typed_param_value_u.l;
- break;
- case VIR_TYPED_PARAM_ULLONG:
- params[dest].value.ul =
- info[src].value.remote_typed_param_value_u.ul;
- break;
- case VIR_TYPED_PARAM_DOUBLE:
- params[dest].value.d =
- info[src].value.remote_typed_param_value_u.d;
- break;
- case VIR_TYPED_PARAM_BOOLEAN:
- params[dest].value.b =
- info[src].value.remote_typed_param_value_u.b;
- break;
- case VIR_TYPED_PARAM_STRING:
- params[dest].value.s =
- strdup(info[src].value.remote_typed_param_value_u.s);
- if (params[dest].value.s == NULL)
- goto out_of_memory;
- break;
- default:
- remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
- params[dest].type);
- goto cleanup;
- }
- }
+ memset(params, 0, sizeof(*params) * nparams * ncpus);
+ for (cpu = 0; cpu < ncpus; cpu++) {
+ int tmp = nparams;
+ remote_typed_param *stride = &ret.params.params_val[cpu *
ret.nparams];
+
+ if (remoteDeserializeTypedParameters(stride, ret.nparams,
+ REMOTE_NODE_CPU_STATS_MAX,
+ ¶ms[cpu * nparams],
+ &tmp) < 0)
+ goto cleanup;
}
rv = ret.nparams;
@@ -2415,9 +2398,6 @@ cleanup:
done:
remoteDriverUnlock(priv);
return rv;
-out_of_memory:
- virReportOOMError();
- goto cleanup;
}
diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x
index 0bd399c..b58925a 100644
--- i/src/remote/remote_protocol.x
+++ w/src/remote/remote_protocol.x
@@ -209,8 +209,13 @@ const REMOTE_DOMAIN_SEND_KEY_MAX = 16;
const REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX = 16;
/*
- * Upper limit on list of (real) cpu parameters
- * nparams(<=16) * ncpus(<=128) <= 2048.
+ * Upper limit on cpus involved in per-cpu stats
+ */
+const REMOTE_DOMAIN_GET_CPU_STATS_NCPUS_MAX = 128;
+
+/*
+ * Upper limit on list of per-cpu stats:
+ * REMOTE_NODE_CPU_STATS_MAX * REMOTE_DOMAIN_GET_CPU_STATS_MAX
*/
const REMOTE_DOMAIN_GET_CPU_STATS_MAX = 2048;
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list