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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> 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. 

>  
> 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. 
>  

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