Re: [libvirt] [PATCH V4 2/5] remote handler for virDomainGetCPUStats()

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: