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

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

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