Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Thanks for your attention to my earlier mail.  I'll delete all the
comments where we agree :-).

> > > > > +/* bind/unbind usb device interface */  
> > > > > +static int unbind_usb_intf(libxl__gc *gc, char *intf, char 
> > > > > **drvpath)  
> > > > > +{  
> > ... 
> > > > > +        dp = libxl__zalloc(gc, PATH_MAX);  
> > > > > +        dp = realpath(spath, dp);  
> > > >   
> > > > Why is this call to realpath needed ? 
> > >  
> > > In sysfs, /driver sometimes is a link, since we need to save the original 
> > > driver to xenstore, so need to get the realpath of the driver. 
> >  
> > I mean, why is the path with all the symlinks in it not suitable for 
> > storage and later use ? 
> 
> The symlink might be "../../../../../../bus/usb/drivers/btusb", we
> couldn't save that to xenstore for later usage.

So the real reason is simply that it is a relative path and we need an
absolute one, because a relative path is not useful ?

OK, thanks for the explanation.  (I'm not sure whether this is
unobvious enough to warrant a comment, perhaps
 /* sysfs can produce relative paths */
or something.)

> > > > I think you probably do things in the wrong order here.  You should  
> > > > write the old driver info to xenstore first, so that if the bind to  
> > > > usbback fails, you have the old driver info.  
> > >  
> > > Perhaps not. Once we finished adding entries to xenstore, pvusb 
> > > frontend/backend drivers will detect the change and do probe work, if 
> > > USB device is still not bound to USBBACK, there might be error. 
> >  
> > What I mean is this: 
> >  
> > Is it not possible to write the original path to xenstore before doing 
> > the unbind ?  Otherwise it seems like there could be error paths where 
> > the original path is not recorded, the xenstore write fails, and then 
> > the information about how to rebind to the original driver has been 
> > lost. 
> 
> I see. 

Right.  Do you think this warrants a change to the code ?

> > Does writing USBBACK_INFO_PATH"/%s/%s/driver_path" really trigger 
> > usbback ? 
> 
> No, writing driver_path info to xenstore won't trigger usbback. Writing
> frontend/backend info will.

Jolly good.

> > > If we merge libxl__device_usb_add and do_usb_add, then it's cleaner. 
> >  
> > See my comment below.  You've explained the distinction to my 
> > satisfaction. 
> >  
> > But, to solve the duplication of the controller info acquisition, 
> > perhaps you could have do_usb_add take the controller info as a 
> > paramaeter ?
> 
> Yes, could be. Only do_usb_add and do_usb_remove parameters are not
> symmetrical. 

I don't think that matters.  (If it did it would be better to add an
unused parameter to the remove function.)

Regards,
Ian.

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