Re: [PATCH V7 3/7] libxl: add pvusb API

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-10-08 at 15:41 +0100, Ian Jackson wrote:

> > +libxl_device_usbctrl *
> > +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> > +{
> 
> This function should return an rc, and the list should come in an out
> parameter.

For better of worse libxl.h defines the general form of a device API to
include list looking like this (search for "Querying").

If this were for an actual device I'd be adamant that it should follow that
pattern.

Since this is part of a new "controller" abstraction we do in theory have
the freedom to do things differently, but it seems to me that having
something as basic as the list operation differ for devices vs. controller
would do more harm than good even if the controller interface is strictly
better. IOW I'm willing to be convinced otherwise but right now I'm pretty
sure how it is above is the preferable extension to our API.

> > +            const char *tmp, *be_path;
> > +            const char *fe_path = GCSPRINTF("%s/%s", path, *dir);
> > +
> > +            libxl_device_usbctrl_init(usbctrl);
> > +            usbctrl->devid = atoi(*dir);
> > +
> > +            tmp = READ_FRONTEND(gc, "backend-id");
> > +            if (!tmp) goto outerr;
> > +            usbctrl->backend_domid = atoi(tmp);
> > +
> > +            tmp = READ_BACKEND(gc, "usb-ver");
> > +            if (!tmp) goto outerr;
> > +            usbctrl->version = atoi(tmp);
> > +
> > +            tmp = READ_BACKEND(gc, "num-ports");
> > +            if (!tmp) goto outerr;
> > +            usbctrl->ports = atoi(tmp);
> 
> There are 4 nearly identical stanzas here.  I think a more
> comprehensive would be helpful.  Maybe a global macro READ_SUBPATH_INT
               ^MACRO?

Ian.