[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API
>>> On 1/20/2016 at 12:56 PM, in message <569F83F5020000660009E6AC@xxxxxxxxxxxxxxxxxxxxxxx>, "Chun Yan Liu" <cyliu@xxxxxxxx> wrote: > >>>> On 1/19/2016 at 11:48 PM, in message > <22174.23240.402164.635740@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson > <Ian.Jackson@xxxxxxxxxxxxx> wrote: > > 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) > Yes, it's true. > > > - the interface bound to the wrong driver > No, it won't be bound to 'wrong' driver, only maybe not bound to any driver > (Already unbound from usbback, but failed to rebound to its original > driver). > In this case, we would report warning: failed to rebind to driver xxx. > > > And then there is no way for the user to get libxl to re-attempt the > > operation, or clean up. Am I right ? > > Yes. No way to re-attempt usbdev-detach or cleanup driver path in > xenstore. But won't affect next time usbdev-attach the same device. > > > > > 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. > > I'm afraid if the side effect is acceptable? In my testing, some USB > bluetooth > device always fails to rebind to 'btusb' driver after it's unbound > from 'usbback'. In this case, we can't detach it from the domain then. Ian J., any opinion on this? If it's still thought to be better, I'll update patch. > > > > > 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. bind_usbintf already has parameter to indicate which driver to be bound to. - Chunyan > > > > 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. > > > > Sorry, will double check again. > > > > > > +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. > > After checking the codes, yes, except for open and close fd, > libxl_write_exactly does the work. Will reuse it. > > > > > > > > +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. > > Will update. > > Thanks, > Chunyan > > > > > > > Thanks, > > Ian. > > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |