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

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



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


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