Re: [Xen-devel] [PATCH v10 6/6] tools: enable Cache QoS Monitoring feature for libxl/libxc

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-03-28 at 00:53 +0000, Xu, Dongxiao wrote:
> > -----Original Message-----
> > From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> > Sent: Thursday, March 27, 2014 11:14 PM
> > To: Xu, Dongxiao
> > Cc: xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; JBeulich@xxxxxxxx;
> > Ian.Jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> > andrew.cooper3@xxxxxxxxxx; konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx
> > Subject: Re: [PATCH v10 6/6] tools: enable Cache QoS Monitoring feature for
> > libxl/libxc
> > 
> > On Wed, 2014-03-26 at 14:35 +0800, Dongxiao Xu wrote:
> > [...]
> > > +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, libxl_pqos_type
> > qos_type);
> > > +int libxl_pqos_detach(libxl_ctx *ctx, uint32_t domid, libxl_pqos_type
> > qos_type);
> > > +libxl_cqminfo * libxl_getcqminfo(libxl_ctx *ctx, unsigned int *l3c_total,
> > > +    unsigned int *nr_domains, unsigned int *nr_rmids, unsigned int
> > *nr_sockets);
> > [...]
> > > +libxl_cqminfo = Struct("cqminfo", [
> > > +    ("valid",         uint32),
> > > +    ("l3c",           uint64),
> > > +    ])
> > 
> > libxl_getcqminfo seems to have a strange mixture of returning an info
> > struct and returning things via pointers passed as integers. why is
> > that?
> 
> The returned info is a unit data structure containing the L3 cache usage data 
> for certain RMID on certain socket.
> These pointers passed integers tell caller the number of such units are 
> returned.
> 
> This follows one existing function:
> libxl_dominfo * libxl_list_domain(libxl_ctx *ctx, int *nb_domain_out);
> where the return value is the unit dominfo data structure, and *nb_domain_out 
> tells caller how many data structure units are returned.

But libxl_getqminfo has three such nr_* pointers as parameters, which
one is the length of the returned array? What are the other two? How
does one enumerate the things which the other nr_* refer to?

> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index 8389468..664a34d 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -7381,6 +7381,115 @@ out:
> > >      return ret;
> > >  }
> > >
> > > +int main_pqosattach(int argc, char **argv)
> > > +{
> > > +    uint32_t domid;
> > > +    int opt, rc;
> > > +    libxl_pqos_type qos_type;
> > > +
> > > +    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-attach", 2) {
> > > +        /* No options */
> > > +    }
> > > +
> > > +    /* libxl_pqos_attach will handle the parameter check. */
> > > +    libxl_pqos_type_from_string(argv[optind], &qos_type);
> > 
> > This can fail, I think you need to check the return value.
> 
> Yes, I am aware of that.
> The following libxl_pqos_attach() function will check the parameter 
> correctness.
> Actually I put a comment in above (/* libxl_pqos_attach will handle the 
> parameter check. */).

If libxl_pqos_type_from_string fails then qos_type remains undefined, so
you can't rely on some later function checking for correctness, the
undefined value may happen to appear valid.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel