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

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

On Wed, 2015-09-30 at 18:55 +0100, George Dunlap wrote:

> > +int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
> > +                                  uint32_t domid,
> > +                                  int devid,
> > +                                  libxl_device_usbctrl *usbctrl)
> > +{
> > +    GC_INIT(ctx);
> > +    libxl_device_usbctrl *usbctrls;
> > +    int nb = 0;
> > +    int i, rc = -1;
> > +
> > +    usbctrls = libxl_device_usbctrl_list(ctx, domid, &nb);
> > +    if (!nb) goto out;
> > +
> > +    for (i = 0; i < nb; i++) {
> > +        if (devid == usbctrls[i].devid) {
> > +            *usbctrl = usbctrls[i];
> 
> libxl maintainers: Is this kind of copying OK?
> 
> The analog functions for vtpm and nic both take very different
> approaches; both call libxl_device_[type]_init() and then fill in
> individual elements (albeit in different ways).

That depends on the memory lifecycle situation of usbctrls[i] and *usbctrl
vis-a-vis the gc and when they are frred.

Cut out of the context here is a 
+    libxl_device_usbctrl_list_free(usbctrls, nb);

which is going to free any of the pointers in usbctrls[i] which have been
copied to usbctrl. So in this case no it is not ok.

You can't avoid the libxl_device_usbctrl_list_free, since you don't want to
leak all the other elements on the list, so copying seems to be the way to
go.

The IDL should have generated a copy function which can be used (by the
vtpm one too, but it predates the IDL making such things I think).

> 
> > +/*
> > + * USB add
> > + */
> > +static int do_usb_add(libxl__gc *gc, uint32_t domid,
> > +                      libxl_device_usb *usbdev,
> > +                      libxl__ao_device *aodev)
> > +{
> > +    libxl_ctx *ctx = CTX;
> > +    libxl_usbctrlinfo usbctrlinfo;
> > +    libxl_device_usbctrl usbctrl;
> > +    int rc;
> > +
> > +    libxl_usbctrlinfo_init(&usbctrlinfo);
> > +    usbctrl.devid = usbdev->ctrl;
> > +    rc = libxl_device_usbctrl_getinfo(ctx, domid, &usbctrl,
> > &usbctrlinfo);
> > +    if (rc)
> > +        goto out;
> > +
> > +    switch (usbctrlinfo.type) {
> > +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> > +        LOG(ERROR, "Not supported");
> > +        break;
> > +    case LIBXL_USBCTRL_TYPE_PV:
> > +        rc = libxl__device_usb_add_xenstore(gc, domid, usbdev,
> > +                                            aodev->update_json);
> > +        if (rc) goto out;
> > +
> > +        rc = usbback_dev_assign(gc, usbdev);
> 
> This assumes that the usb controller is dom0; but the interface
> explicitly allows pvusb driver domains.
> 
> I think it would be enough here to do this check if the usbctrl device
> is dom0.  We should then assume that a pvusb driver domain will be
> configured with all usb devices assigned to usbback already.
> 
> I assume that there's a feedback mechanisms for backends for situations
> where the requested device can't be made, right?  For example, if you
> have a network driver domain and request a non-existent bridge?  If so,
> I think we can let the same mechanism worth for pvusb backends to say "I
> don't have that device available".

I think the b/e writes an error node in xenstore, which we already pickup
iirc.