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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mer, 2013-12-04 at 02:44 +0000, Xu, Dongxiao wrote:
> > -----Original Message-----
> > From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]

> > Would it be possible to split this patch in 3, one for libxc, one for
> > libxl and one for xl?
> 
> Originally the patch is split (by functional), and later I merged them 
> according to Andrew's suggestion.
> I think merge is okay since the logic is simple and straightforward.
> 
Oh, sorry for not noticing that. My personal preference is to always
split, even if logic is trivial, but again, that's just me. :-)

> > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > > index c7dceda..fdca92d 100644
> > > --- a/tools/libxl/libxl.h
> > > +++ b/tools/libxl/libxl.h
> > > @@ -285,6 +285,7 @@
> > >
> > >  #include <libxl_uuid.h>
> > >  #include <_libxl_list.h>
> > > +#include <xenctrl.h>
> > >
> > Is this really necessary? I think it shouldn't... <xenctrl.h> is already
> > included in "libxl_internal.h", which you are including yourself below,
> > so...
> 
> libxl.h will reference "sysctl_cqminfo_t", which is defined in xenctrl.h.
> I didn't see libxl_internal.h is included in libxl.h, can you help to point 
> it out?
> 
I see. No, all I said is that your new file, libxl_pqos.c, already
includes xenctrl.h, and hence I was asking whether that is not enough
and if not, why, and you just answered.

TBH, it still looks wrong to me. It does not happen for any other
similar situations and data type (or at least situations and data type
that look similar enough to me). What we do there, is defining a libxl
counterpart of the xc_* type, use it in the libxl interface and
translate between the twos _inside_ the libxl function, which is
implemented somewhere where libxl_internal.h, and then xenctrl.h, is
included, and have no problem seeing the xc_* type declaration.

Look, for example at libxl_get_physinfo(), xc_physinfo(), xc_physinfo_t.

Or you think your case is somewhat different? If yes, how?

> > > +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, const char * 
> > > qos_type)
> > > +{
> > > +    int rc;
> > > +    uint32_t flags = 0;
> > > +
> > > +    if (!strncmp(qos_type, "cqm", 3))
> > > +        flags |= XEN_DOMCTL_pqos_cqm;
> > > +    else {
> > > +        rc = -EINVAL;
> > > +        LIBXL__LOG(ctx, XTL_ERROR, "%s", msg[EINVAL]);
> > >
> > I think new code should use the LOG() / LOGE() variant of the logging
> > macros.
> 
> I saw a lot of existing code still uses LIBXL__LOG() function.
> Besides, if we use LOGE(), we need to pass another variable "gc" to the 
> function...
> Is this a guideline that we will stick to LOG()/LOGE() and deprecate calling 
> of LIBXL__LOG() in later code?
> 
I don't think it's written anywhere yet, but I'm not sure. Anyway, I've
seen similar requests on this list (even got one! :-P) from some time
now.

As per the new gc parameter, that's not true, all you need is adding a
GC_INIT(ctx); call at the beginning of the function and a GC_FREE at the
end.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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