On Sat, 28 Jan 2012 11:16:03 -0700 Eric Blake <eblake@redhat.com> wrote:
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.
Ok.
+ /* 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.
Sorry.
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.
Yes. you're right.
Oh, I just realized that to support string entries, we have to support a flags of VIR_TYPED_PARAM_STRING_OKAY.
Ah, I missed that.
+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.
Ah, skip is not
+++ 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:
seems nicer ! Thank you for kindly reviewing. Thanks, -Kame