Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Mar 3, 2016 at 3:11 AM, Chun Yan Liu <cyliu@xxxxxxxx> wrote:
>>>> On 3/3/2016 at 02:32 AM, in message <56D731B1.60009@xxxxxxxxxx>, George 
>>>> Dunlap
> <george.dunlap@xxxxxxxxxx> wrote:
>> On 01/03/16 08:09, Chunyan Liu wrote:
>> > +static int usbdev_rebind(libxl__gc *gc, const char *busid)
>> > +{
>> > +    char **intfs = NULL;
>> > +    char *usbdev_encode = NULL;
>> > +    char *path = NULL;
>> > +    int i, num = 0;
>> > +    int rc;
>> > +
>> > +    rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num);
>> > +    if (rc) goto out;
>> > +
>> > +    usbdev_encode = usb_interface_xenstore_encode(gc, busid);
>> > +
>> > +    for (i = 0; i < num; i++) {
>> > +        char *intf = intfs[i];
>> > +        char *usbintf_encode = NULL;
>> > +        const char *drvpath;
>> > +
>> > +        /* rebind USB interface to its originial driver */
>> > +        usbintf_encode = usb_interface_xenstore_encode(gc, intf);
>> > +        path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path",
>> > +                         usbdev_encode, usbintf_encode);
>> > +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath);
>> > +        if (rc) goto out;
>> > +
>> > +        if (drvpath) {
>> > +            rc = bind_usbintf(gc, intf, drvpath);
>> > +            if (rc) {
>> > +                LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath);
>> > +                goto out;
>> > +            }
>> > +        }
>> > +    }
>> > +
>> > +    path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode);
>> > +    rc = libxl__xs_rm_checked(gc, XBT_NULL, path);
>> > +
>> > +out:
>>
>> So it looks like if one of the re-binds fails, then it stops where it is
>> and leaves the USBBACK re-bind info in xenstore.  In that case it's not
>> clear to me how that information would ever be removed.
>>
>> I think until such time as we have a command to re-attempt the re-bind,
>>  if there's an error in the actual rebind, it should just break out of
>> the for loop, and remove the re-bind nodes, and document a way to let
>> the user try to clean things up.
>
> Just according to last time discussion about how to handle the rebind
> failure, seems Ian preferred to add a xl command to deal with rebind
> in future, based on that need, I think driver_path info should be kept
> in xenstore then. Without that need, I agree it's best to remove
> xenstore nodes. So, keep or remove?

Well when we have such a command, then yes, we'll need to keep the
nodes for it to use and delete.  But at the moment, we have no such
command, so these nodes will just sit around cluttering up the libxl
xenstore space, and nothing will delete them (unless a user manually
runs xenstore-rm).

So I think for now we should delete them on failure; and at some point
later, when someone implements a recovery command, then they should
change this code to not delete the xenstore nodes (and also change the
log messages to point to that command).

 -George

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