Re: [PATCH V13 3/5] libxl: add pvusb API

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"):
> Add pvusb APIs, including:
>  - attach/detach (create/destroy) virtual usb controller.
>  - attach/detach usb device
>  - list usb controller and usb devices
>  - some other helper functions


Thanks.  This is making progress but I'm afraid we're not quite there
yet.


> +static int usbback_dev_unassign(libxl__gc *gc, const char *busid)
> +{
...
> +    /* Till here, USB device has been unbound from USBBACK and
> +     * removed from xenstore, usb list couldn't show it anymore,
> +     * so no matter removimg driver path successfully or not,
> +     * we will report operation success.
> +     */

I'm still unconvinced by this and this may mean that the code in this
function is in the wrong order.  Earlier we had this exchange:

> > Ought this function to really report success if these calls fail ?
> 
> I think so. Till here, the USB device has already been unbound from
> usbback and removed from xenstore. usb-list cannot list it any more.

The problem is that I think that if this function fails, it can leave
 - debris in xenstore (the usbback path)
 - the interface bound to the wrong driver
And then there is no way for the user to get libxl to re-attempt the
operation, or clean up.  Am I right ?

One way to avoid this kind of problem is to deal with the xenstore
path last.  That way the device will still appear as attached to the
domain.

To do this properly I think bind_usbintf may need to become idempotent
even in the face of other callers racing to assign the device.  I
think that would mean bind_usbif it would have to know what driver to
expect to find the device bound to.

George, am I right here ?


I have a few other comments too:

> +/* get original driver path of usb interface, stored in @drvpath */
> +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char **drvpath)
> +{
> +    char *spath, *dp = NULL;
> +    struct stat st;
> +    int rc;
> +
> +    spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf);
> +
> +    rc = lstat(spath, &st);

This `rc' should be `r'.  (CODING_STYLE.)

I mentioned this in my review of v12 in the context of
sysfs_write_intf.  But there is still more than one occurrence in v13
of `rc' for a syscall return value.


> +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char *path)
> +{

Last time I wrote:

  But there is nothing usb specific about this function.  Looking for
  similar code elsewhere this function seems very similar to
  libxl_pci.c:sysfs_write_bdf, but I won't ask you to try to refactor
  these two functions together.

This time I have remembered that libxl_write_exactly exists, and could
be used.  Sorry for not saing this last time but I think you can
probably just get rid of this in favour of libxl_write_exactly.  If
you think not, please say why.


> +static int bind_usbintf(libxl__gc *gc, const char *intf, const char *drvpath)
> +{
> +    char *path;
> +    struct stat st;
> +
> +    path = GCSPRINTF("%s/%s", drvpath, intf);
> +    /* if already bound, return */
> +    if (!lstat(path, &st))
> +        return 0;
> +    else if (errno != ENOENT)
> +        return ERROR_FAIL;

This new ERROR_FAIL fails to log anything, and probably should.  I
think the anomalous style leads to this mistake.  You should say
       r = lstat...
and adopt the pattern from the rest of libxl.


Thanks,
Ian.