On Sat, 28 Jan 2012 11:16:03 -0700
Eric Blake <eblake@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list