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

[George Dunlap]

On 19/11/15 06:24, Chun Yan Liu wrote:
> 
> 
 On 11/19/2015 at 09:33 AM, in message
> <564d9761026600085...@relay2.provo.novell.com>, "Chun Yan Liu"
>  wrote: 
> 
>>  
> On 11/18/2015 at 05:44 PM, in message <20151118094410.gb21...@aepfle.de>, 
> Olaf 
>> Hering  wrote:  
>>> On Tue, Nov 17, Chun Yan Liu wrote:  
>>>   
 I think libxl_device_usb doesn't need to be changed into   
>>> libxl_device_usbdev?   
> 
> George & Olaf,
> 
> About the naming, can we get to a decision?
> e.g.
> * usb controller and everything related, using "usbctrl"
> * usb device and everything related, using "usbdev" (?)
> Currently in pvusb, almost everywhere referring to a usb device, we use "usb".
> Like: libxl_device_usb, libxl_device_usb_add/remove, etc.
> 
> If we decide, I can update all together.

So I finally went back and spent some time mulling over the e-mail
thread we had before about designing the interface.

Just as a reminder, SCSI has a topology of host / bus / target / LUN;
USB has a topology controller / hub[bus] / [port]device / interface.

At the moment in USB we're not dealing with virtual hubs, so each
controller will have a single bus.  Additionally, we are only passing
through and/or creating devices; a physical device with more than one
interface will have all of its interfaces passed through transparently.
 So we're only effectively exposing two levels in our API.

In the previous discussion, for pvscsi, Juergen argued it makes sense to
be able to assign LUNs from the same physical target to different
virtual targets; and it also makes sense to be able to assign
newly-created physical LUNs to existing virtual targets.

(Please people correct me if I've misunderstood something anywhere.)

So for SCSI, there may be three levels at which people want to be able
to do things.

Given, that, I wonder if it would make sense to name the different
"levels" for these multi-level devices after the name for that level in
their respective specificaitons.

I.e., libxl_device_usbctrl, libxl_device_usbdev, libxl_device_scsihost,
libxl_device_scsitgt, libxl_device_scsilun or something like that.

In which case, "usbdev" would be indicated (since it's the "device"
we're talking about).

Thoughts?

 -George

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

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

[Olaf Hering]

On Wed, Nov 18, Ian Campbell wrote:

> I'd been hoping that someone involved ion this would generate a patch
> adding a template for this controller+devices model to libxl.h, I've not
> seen anything since George's original RFC[0] "libxl: Introduce a template
> for devices with a controller".

Its still on my list. So far its not clear to me how it will look like
in the end, depends on what the existing backend drivers expect.

Olaf

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

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

[Olaf Hering]

On Tue, Nov 17, Chun Yan Liu wrote:

> I think libxl_device_usb doesn't need to be changed into libxl_device_usbdev? 

In case of vscsi the struct and functions names are odd. It was not
obvious which one belongs to a ctrl and which one belongs to a device.
In the meantime I have changed everything related to a scsi host, it
contains now 'vscsictrl'. The names related to devices will follow.

I suggest to do apply the same also to usb and make it clear what
belongs to a ctrl and what to a device. Havent checked your patch what
places that actually would be.

Olaf

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

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

[Chun Yan Liu]

>>> On 11/18/2015 at 05:44 PM, in message <20151118094410.gb21...@aepfle.de>, 
>>> Olaf
Hering  wrote: 
> On Tue, Nov 17, Chun Yan Liu wrote: 
>  
> > I think libxl_device_usb doesn't need to be changed into  
> libxl_device_usbdev?  
>  
> In case of vscsi the struct and functions names are odd. It was not 
> obvious which one belongs to a ctrl and which one belongs to a device. 
> In the meantime I have changed everything related to a scsi host, it 
> contains now 'vscsictrl'. The names related to devices will follow. 
>  
> I suggest to do apply the same also to usb and make it clear what 
> belongs to a ctrl and what to a device. Havent checked your patch what 
> places that actually would be. 

Currently in pvusb patches, all places using 'usbctrl' means usb controller,
'usb' means usb device.

- Chunyan

>  
> Olaf 
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  



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

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

[Chun Yan Liu]

>>> On 11/19/2015 at 09:33 AM, in message
<564d9761026600085...@relay2.provo.novell.com>, "Chun Yan Liu"
 wrote: 

>  
 On 11/18/2015 at 05:44 PM, in message <20151118094410.gb21...@aepfle.de>, 
 Olaf 
> Hering  wrote:  
> > On Tue, Nov 17, Chun Yan Liu wrote:  
> >   
> > > I think libxl_device_usb doesn't need to be changed into   
> > libxl_device_usbdev?   

George & Olaf,

About the naming, can we get to a decision?
e.g.
* usb controller and everything related, using "usbctrl"
* usb device and everything related, using "usbdev" (?)
Currently in pvusb, almost everywhere referring to a usb device, we use "usb".
Like: libxl_device_usb, libxl_device_usb_add/remove, etc.

If we decide, I can update all together.

- Chunyan

> >   
> > In case of vscsi the struct and functions names are odd. It was not  
> > obvious which one belongs to a ctrl and which one belongs to a device.  
> > In the meantime I have changed everything related to a scsi host, it  
> > contains now 'vscsictrl'. The names related to devices will follow.  
> >   
> > I suggest to do apply the same also to usb and make it clear what  
> > belongs to a ctrl and what to a device. Havent checked your patch what  
> > places that actually would be.  
>  
> Currently in pvusb patches, all places using 'usbctrl' means usb controller, 
> 'usb' means usb device. 
>  
> - Chunyan 
>  
> >   
> > Olaf  
> >   
> > ___  
> > Xen-devel mailing list  
> > Xen-devel@lists.xen.org  
> > http://lists.xen.org/xen-devel  
> >   
> >   
>  
>  
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  



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

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

[Chun Yan Liu]

>>> On 11/16/2015 at 06:01 PM, in message <5649a988.7010...@citrix.com>, George
Dunlap  wrote: 
> On 13/11/15 11:19, Olaf Hering wrote: 
> > On Wed, Oct 21, Chunyan Liu wrote: 
> >  
> >> Add pvusb APIs, including: 
> >  
> >> @@ -635,6 +664,8 @@ libxl_domain_config = Struct("domain_config", [ 
> >>  ("pcidevs", Array(libxl_device_pci, "num_pcidevs")), 
> >>  ("rdms", Array(libxl_device_rdm, "num_rdms")), 
> >>  ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")), 
> >> +("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")), 
> >> +("usbs", Array(libxl_device_usb, "num_usbs")), 
> >  
> > Should that perhaps be "usbctrls" + "usbdevs" if the latter really 
> > represents a device below one of the "usbctrls"? I dont know if it does, 
> > just wondering. 
>  
> That seems like a reasonable suggestion. 

OK. I'll update "usbs" to "usbdevs".
I think libxl_device_usb doesn't need to be changed into libxl_device_usbdev? 

>  
>  -George 
>  
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  



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

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

[Ian Jackson]

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@lists.xen.org
http://lists.xen.org/xen-devel

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

[Chun Yan Liu]

>>> On 11/17/2015 at 02:06 AM, in message
<22090.6954.35639.703...@mariner.uk.xensource.com>, Ian Jackson
 wrote: 
> 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.) 

Take all. Will update codes.

>  
> Regards, 
> Ian. 
>  
> ___ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>  



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

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

[George Dunlap]

On 13/11/15 11:19, Olaf Hering wrote:
> On Wed, Oct 21, Chunyan Liu wrote:
> 
>> Add pvusb APIs, including:
> 
>> @@ -635,6 +664,8 @@ libxl_domain_config = Struct("domain_config", [
>>  ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
>>  ("rdms", Array(libxl_device_rdm, "num_rdms")),
>>  ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
>> +("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
>> +("usbs", Array(libxl_device_usb, "num_usbs")),
> 
> Should that perhaps be "usbctrls" + "usbdevs" if the latter really
> represents a device below one of the "usbctrls"? I dont know if it does,
> just wondering.

That seems like a reasonable suggestion.

 -George


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

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

[Olaf Hering]

On Wed, Oct 21, Chunyan Liu wrote:

> Add pvusb APIs, including:

> @@ -635,6 +664,8 @@ libxl_domain_config = Struct("domain_config", [
>  ("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
>  ("rdms", Array(libxl_device_rdm, "num_rdms")),
>  ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
> +("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
> +("usbs", Array(libxl_device_usb, "num_usbs")),

Should that perhaps be "usbctrls" + "usbdevs" if the latter really
represents a device below one of the "usbctrls"? I dont know if it does,
just wondering.


Olaf

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

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

[Olaf Hering]

On Wed, Oct 21, Chunyan Liu wrote:

> Add pvusb APIs, including:

Some comments below.

After a quick look I miss the proposed ctrl/device separation for pvscsi
(what handles "state" changes?). But, I have to read all the other
dozen+ threads about that topic first.


> +flexarray_append_pair(back, "state", "1");

4.6+ has macros for "state" values, like
flexarray_append_pair(back, "state", GCSPRINTF("%d", XenbusStateInitialising));


> +flexarray_append_pair(front, "state", "1");

4.6+ has macros for "state" values.

> +LOG(DEBUG, "Adding new usb device to xenstore");

Which one? Perhaps print also details.

> +LOG(DEBUG, "Removing USB device from xenstore");

Which one? Perhaps print also details.

> +/* check if the USB interface is already bound to "usbbcak" */

Typo.


Olaf

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

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

[Chun Yan Liu]

>>> On 11/13/2015 at 01:27 AM, in message
, George
Dunlap  wrote: 
> On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu  wrote: 
> > +static int 
> > +get_assigned_devices(libxl__gc *gc, 
> > + libxl_device_usb **list, int *num) 
> > +{ 
> > +char **domlist; 
> > +unsigned int nd = 0, i, j, k; 
> > +int rc; 
> > + 
> > +*list = NULL; 
> > +*num = 0; 
> > + 
> > +domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", ); 
> > +for (i = 0; i < nd; i++) { 
> > +char *path, **ctrl_list; 
> > +unsigned int nc = 0; 
> > + 
> > +path = GCSPRINTF("/local/domain/%s/device/vusb", domlist[i]); 
> > +ctrl_list = libxl__xs_directory(gc, XBT_NULL, path, ); 
> > + 
> > +for (j = 0; j < nc; j++) { 
> > +const char *be_path, *num_ports; 
> > +int nport; 
> > + 
> > +rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +  GCSPRINTF("%s/%s/backend", path, ctrl_list[j]), 
> > +  _path); 
> > +if (rc) goto out; 
> > + 
> > +rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +GCSPRINTF("%s/num-ports", 
> > be_path), 
> > +_ports); 
> > +if (rc) goto out; 
> > + 
> > +nport = atoi(num_ports); 
> > +for (k = 0; k < nport; k++) { 
> > +libxl_device_usb *usb; 
> > +const char *portpath, *busid; 
> > + 
> > +portpath = GCSPRINTF("%s/port/%d", be_path, k + 1); 
> > +busid = libxl__xs_read(gc, XBT_NULL, portpath); 
> > +/* If there is USB device attached, add it to list */ 
> > +if (busid && strcmp(busid, "")) { 
> > +GCREALLOC_ARRAY(*list, *num + 1); 
> > +usb = *list + *num; 
> > +(*num)++; 
> > +libxl_device_usb_init(usb); 
> > +usb->ctrl = atoi(ctrl_list[j]); 
> > +usb->port = k + 1; 
> > +rc = usb_busaddr_from_busid(gc, busid, 
> > +>u.hostdev.hostbus, 
> > +>u.hostdev.hostaddr); 
>  
> You should probably set usb->devtype to HOSTDEV here, even though this 
> function is internal. 
>  
> > +if (rc) goto out; 
> > +} 
> > +} 
> > +} 
> > +} 
> > + 
> > +rc = 0; 
> > + 
> > +out: 
> > +if (rc) { 
> > +*list = NULL; 
> > +*num = 0; 
> > +} 
> > +return rc; 
> > +} 
> > + 
> > +static bool is_usbdev_in_array(libxl_device_usb *usbs, int num, 
> > +   libxl_device_usb *usb) 
> > +{ 
> > +int i; 
> > + 
> > +for (i = 0; i < num; i++) { 
> > +if (usbs[i].u.hostdev.hostbus == usb->u.hostdev.hostbus && 
> > +usbs[i].u.hostdev.hostaddr == usb->u.hostdev.hostaddr) 
> > +return true; 
> > +} 
> > + 
> > +return false; 
> > +} 
> > + 
> > +/* check if USB device is already assigned to a domain */ 
> > +/* check if USB device type is assignable */ 
> > +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb) 
> > +{ 
> > +int classcode; 
> > +char *filename; 
> > +void *buf = NULL; 
> > +char *busid = NULL; 
> > + 
> > +busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, 
> > + usb->u.hostdev.hostaddr); 
> > +if (!busid) return false; 
> > + 
> > +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid); 
> > +if (libxl__read_sysfs_file_contents(gc, filename, , NULL)) 
> > +return false; 
> > + 
> > +classcode = atoi(buf); 
> > +return classcode != USBHUB_CLASS_CODE; 
> > +} 
> > + 
> > +/* get usb devices under certain usb controller */ 
> > +static int 
> > +libxl__device_usb_list_for_usbctrl(libxl__gc *gc, uint32_t domid, 
> > +   libxl_devid usbctrl, 
> > +   libxl_device_usb **usbs, int *num) 
> > +{ 
> > +const char *fe_path, *be_path, *num_devs; 
> > +int n, i, rc; 
> > + 
> > +*usbs = NULL; 
> > +*num = 0; 
> > + 
> > +fe_path = GCSPRINTF("%s/device/vusb/%d", 
> > +libxl__xs_get_dompath(gc, domid), usbctrl); 
> > + 
> > +rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +GCSPRINTF("%s/backend", fe_path), 
> > +_path); 
> > +if (rc) return rc; 
> > + 
> > +rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +GCSPRINTF("%s/num-ports", be_path), 
> > +_devs); 
> > +if (rc) return rc; 
> > + 
> > +

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

[Chun Yan Liu]

>>> On 11/12/2015 at 07:32 PM, in message <20151112113200.ga...@aepfle.de>, Olaf
Hering  wrote: 
> On Wed, Oct 21, Chunyan Liu wrote: 
>  
> > Add pvusb APIs, including: 
>  
> Some comments below. 
>  
> After a quick look I miss the proposed ctrl/device separation for pvscsi 
> (what handles "state" changes?). But, I have to read all the other 
> dozen+ threads about that topic first. 
>  
>  
> > +flexarray_append_pair(back, "state", "1"); 
>  
> 4.6+ has macros for "state" values, like 
> flexarray_append_pair(back, "state", GCSPRINTF("%d",  
> XenbusStateInitialising)); 
>  
>  
> > +flexarray_append_pair(front, "state", "1"); 
>  
> 4.6+ has macros for "state" values. 
>  
> > +LOG(DEBUG, "Adding new usb device to xenstore"); 
>  
> Which one? Perhaps print also details. 
>  
> > +LOG(DEBUG, "Removing USB device from xenstore"); 
>  
> Which one? Perhaps print also details. 
>  
> > +/* check if the USB interface is already bound to "usbbcak" */ 
>  
> Typo. 

Take all. Thanks Olaf!

- Chunyan

>  
>  
> Olaf 
>  
>  



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

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

[Chun Yan Liu]

>>> On 11/13/2015 at 01:00 AM, in message
<22084.50631.506663.212...@mariner.uk.xensource.com>, Ian Jackson
 wrote: 
> Chun Yan Liu writes ("Re: [PATCH V8 3/7] libxl: add pvusb API"): 
> > On 11/10/2015 at 02:11 AM, in message 
> > <22080.57829.461049.37...@mariner.uk.xensource.com>, Ian Jackson 
> >  wrote:  
> > > Chunyan Liu writes ("[PATCH V8 3/7] libxl: add pvusb API"):  
> > > > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,  
> > > > +   libxl_device_usbctrl *usbctrl,  
> > > > +   libxl__ao_device *aodev)  
> > > > +{  
> > >   
> > > Thanks for adjusting the error-handling patterns in these functions.  
> > > The new way is good, except that:  
> > >   
> > > > +out:  
> > > > +aodev->rc = rc;  
> > > > +if (rc) aodev->callback(egc, aodev);  
> > >   
> > > Here, rc is always set, and indeed the code would be wrong if it were  
> > > not.  So can you remove the conditional please ?  Ie:  
> >  
> > Reading the codes, libxl__wait_device_connection will call 
> > aodev->callback properly. 
>  
> Indeed.  So you need to call aodev->callback() iff you don't call 
> libxl__wait_device_connection.  But libxl__wait_device_connection is 
> called only on the success exit path which ends in `return', not in 
> `goto out'.  So: 
>  
> > So here, only if (rc != 0), that means 
> > error happens, then we need to call aodev->callback to end the 
> > process. (Refer to current libxl__device_disk_add, all current code 
> > does similar work.) So I think the code is not wrong (?) 
>  
> In the `goto out' path, rc is always set.  Writing `if (rc)' implies 
> that it might not be (or is allowed not to be), which is misleading.

I'm convinced your suggestion is better :-). I'll adjust codes.
 
>  
>  
> > > > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,  
> > > > +  uint8_t *bus, uint8_t *addr)  
> > > > +{  
> > > > +char *filename;  
> > > > +void *buf;  
> > > > +  
> > > > +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);  
> > > > +if (!libxl__read_sysfs_file_contents(gc, filename, , NULL))  
> > > > +*bus = atoi((char *)buf);  
> > >   
> > > I don't think this cast (and the one for addr) are necessary ?  
> >  
> > Which cast? Here, we want to get a uint_8 value, but buf is a string, 
> > we need to do atoi. 
>  
> I mean that I think 
>  
>  +*bus = atoi(buf);  
>  
> would compile and be correct.  buf would be automatically converted 
> from void* to const char*.  It's better to avoid casts where they are 
> not needed, because casts will suppress compiler warnings. 

Yes, you're right. Thanks very much!

>  
> For example if someone wanted to have the buffer be an updated 
> parameter they might do this: 
>  
>   static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,  
> +   void **buf, 
> uint8_t *bus, uint8_t *addr) 
> -void *buf;  
>  
> and hope or expect the compiler to notice places where they had failed 
> to update the usage of buf.  With void **buf, 
>   atoi((char*)buf) 
> would compile and do a wrong thing whereas 
>   atoi(buf) 
> would produce a fatal warning. 
>  
> > > > +/* 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.

>  
>  
> > > > +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb)  
> > > > +{  
> > > ...  
> > > > +/* bind interface to its originial driver */  
> > > > +drvpath = libxl__xs_read(gc, XBT_NULL,  
> > > > +  GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",  
> > > > +  usb_encode, usb_interface_xenstore_encode(intf)));  
> > > > +if (drvpath && bind_usb_intf(gc, intf, drvpath))  
> > > > +LOGE(WARN, "Couldn't bind %s to %s", intf, drvpath);  
> > >   
> > > This error message could be clearer.  
> > >   
> > > Also it would be worth a comment in the code to explain why this is a  
> > > warning (merely logged), rather than an error (logged and reported  
> > > with nonzero status to caller). 
> >  
> > Will update. When doing USB remove, it's the ideal result we can rebound 
> > the USB to its original driver, in case the USB interface failed to be 
> > rebound to 

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

[Ian Jackson]

Chun Yan Liu writes ("Re: [PATCH V8 3/7] libxl: add pvusb API"):
> On 11/10/2015 at 02:11 AM, in message
> <22080.57829.461049.37...@mariner.uk.xensource.com>, Ian Jackson
>  wrote: 
> > Chunyan Liu writes ("[PATCH V8 3/7] libxl: add pvusb API"): 
> > > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> > > +   libxl_device_usbctrl *usbctrl, 
> > > +   libxl__ao_device *aodev) 
> > > +{ 
> >  
> > Thanks for adjusting the error-handling patterns in these functions. 
> > The new way is good, except that: 
> >  
> > > +out: 
> > > +aodev->rc = rc; 
> > > +if (rc) aodev->callback(egc, aodev); 
> >  
> > Here, rc is always set, and indeed the code would be wrong if it were 
> > not.  So can you remove the conditional please ?  Ie: 
> 
> Reading the codes, libxl__wait_device_connection will call
> aodev->callback properly.

Indeed.  So you need to call aodev->callback() iff you don't call
libxl__wait_device_connection.  But libxl__wait_device_connection is
called only on the success exit path which ends in `return', not in
`goto out'.  So:

> So here, only if (rc != 0), that means
> error happens, then we need to call aodev->callback to end the
> process. (Refer to current libxl__device_disk_add, all current code
> does similar work.) So I think the code is not wrong (?)

In the `goto out' path, rc is always set.  Writing `if (rc)' implies
that it might not be (or is allowed not to be), which is misleading.


> > > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, 
> > > +  uint8_t *bus, uint8_t *addr) 
> > > +{ 
> > > +char *filename; 
> > > +void *buf; 
> > > + 
> > > +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid); 
> > > +if (!libxl__read_sysfs_file_contents(gc, filename, , NULL)) 
> > > +*bus = atoi((char *)buf); 
> >  
> > I don't think this cast (and the one for addr) are necessary ? 
> 
> Which cast? Here, we want to get a uint_8 value, but buf is a string,
> we need to do atoi.

I mean that I think

 +*bus = atoi(buf); 

would compile and be correct.  buf would be automatically converted
from void* to const char*.  It's better to avoid casts where they are
not needed, because casts will suppress compiler warnings.

For example if someone wanted to have the buffer be an updated
parameter they might do this:

  static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, 
+   void **buf,
uint8_t *bus, uint8_t *addr)
-void *buf; 

and hope or expect the compiler to notice places where they had failed
to update the usage of buf.  With void **buf,
  atoi((char*)buf)
would compile and do a wrong thing whereas
  atoi(buf)
would produce a fatal warning.

> > > +/* 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 ?


> > > +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb) 
> > > +{ 
> > ... 
> > > +/* bind interface to its originial driver */ 
> > > +drvpath = libxl__xs_read(gc, XBT_NULL, 
> > > +  GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path", 
> > > +  usb_encode, usb_interface_xenstore_encode(intf))); 
> > > +if (drvpath && bind_usb_intf(gc, intf, drvpath)) 
> > > +LOGE(WARN, "Couldn't bind %s to %s", intf, drvpath); 
> >  
> > This error message could be clearer. 
> >  
> > Also it would be worth a comment in the code to explain why this is a 
> > warning (merely logged), rather than an error (logged and reported 
> > with nonzero status to caller).
> 
> Will update. When doing USB remove, it's the ideal result we can rebound
> the USB to its original driver, in case the USB interface failed to be
> rebound to its original driver, we will export warning but won't stop the
> removing process.

Right.  Please put that in a comment :-).


> > > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb) 
> > > +{ 
> > ... 
> > > +if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 
> > > 0) { 
> > > +LOG(WARN, "Write of %s to node %s failed", drvpath, 
> > > path); 
> > > +} 
> >  
> > One of the advantages of libxl__xs_write_checked is that it logs 
> > errors.  So you can leave that log message out. 
> >  
> > However, if the xs write fails, you should presumably stop, rather 
> > than carrying on. 
> > 
> > I think you probably do things 

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

[George Dunlap]

On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu  wrote:
> +static int
> +get_assigned_devices(libxl__gc *gc,
> + libxl_device_usb **list, int *num)
> +{
> +char **domlist;
> +unsigned int nd = 0, i, j, k;
> +int rc;
> +
> +*list = NULL;
> +*num = 0;
> +
> +domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", );
> +for (i = 0; i < nd; i++) {
> +char *path, **ctrl_list;
> +unsigned int nc = 0;
> +
> +path = GCSPRINTF("/local/domain/%s/device/vusb", domlist[i]);
> +ctrl_list = libxl__xs_directory(gc, XBT_NULL, path, );
> +
> +for (j = 0; j < nc; j++) {
> +const char *be_path, *num_ports;
> +int nport;
> +
> +rc = libxl__xs_read_checked(gc, XBT_NULL,
> +  GCSPRINTF("%s/%s/backend", path, ctrl_list[j]),
> +  _path);
> +if (rc) goto out;
> +
> +rc = libxl__xs_read_checked(gc, XBT_NULL,
> +GCSPRINTF("%s/num-ports", be_path),
> +_ports);
> +if (rc) goto out;
> +
> +nport = atoi(num_ports);
> +for (k = 0; k < nport; k++) {
> +libxl_device_usb *usb;
> +const char *portpath, *busid;
> +
> +portpath = GCSPRINTF("%s/port/%d", be_path, k + 1);
> +busid = libxl__xs_read(gc, XBT_NULL, portpath);
> +/* If there is USB device attached, add it to list */
> +if (busid && strcmp(busid, "")) {
> +GCREALLOC_ARRAY(*list, *num + 1);
> +usb = *list + *num;
> +(*num)++;
> +libxl_device_usb_init(usb);
> +usb->ctrl = atoi(ctrl_list[j]);
> +usb->port = k + 1;
> +rc = usb_busaddr_from_busid(gc, busid,
> +>u.hostdev.hostbus,
> +>u.hostdev.hostaddr);

You should probably set usb->devtype to HOSTDEV here, even though this
function is internal.

> +if (rc) goto out;
> +}
> +}
> +}
> +}
> +
> +rc = 0;
> +
> +out:
> +if (rc) {
> +*list = NULL;
> +*num = 0;
> +}
> +return rc;
> +}
> +
> +static bool is_usbdev_in_array(libxl_device_usb *usbs, int num,
> +   libxl_device_usb *usb)
> +{
> +int i;
> +
> +for (i = 0; i < num; i++) {
> +if (usbs[i].u.hostdev.hostbus == usb->u.hostdev.hostbus &&
> +usbs[i].u.hostdev.hostaddr == usb->u.hostdev.hostaddr)
> +return true;
> +}
> +
> +return false;
> +}
> +
> +/* check if USB device is already assigned to a domain */
> +/* check if USB device type is assignable */
> +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
> +{
> +int classcode;
> +char *filename;
> +void *buf = NULL;
> +char *busid = NULL;
> +
> +busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> + usb->u.hostdev.hostaddr);
> +if (!busid) return false;
> +
> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid);
> +if (libxl__read_sysfs_file_contents(gc, filename, , NULL))
> +return false;
> +
> +classcode = atoi(buf);
> +return classcode != USBHUB_CLASS_CODE;
> +}
> +
> +/* get usb devices under certain usb controller */
> +static int
> +libxl__device_usb_list_for_usbctrl(libxl__gc *gc, uint32_t domid,
> +   libxl_devid usbctrl,
> +   libxl_device_usb **usbs, int *num)
> +{
> +const char *fe_path, *be_path, *num_devs;
> +int n, i, rc;
> +
> +*usbs = NULL;
> +*num = 0;
> +
> +fe_path = GCSPRINTF("%s/device/vusb/%d",
> +libxl__xs_get_dompath(gc, domid), usbctrl);
> +
> +rc = libxl__xs_read_checked(gc, XBT_NULL,
> +GCSPRINTF("%s/backend", fe_path),
> +_path);
> +if (rc) return rc;
> +
> +rc = libxl__xs_read_checked(gc, XBT_NULL,
> +GCSPRINTF("%s/num-ports", be_path),
> +_devs);
> +if (rc) return rc;
> +
> +n = atoi(num_devs);
> +
> +for (i = 0; i < n; i++) {
> +char *busid;
> +libxl_device_usb *usb;
> +
> +busid = libxl__xs_read(gc, XBT_NULL,
> +   GCSPRINTF("%s/port/%d", be_path, i + 1));
> +if (busid && strcmp(busid, "")) {
> +GCREALLOC_ARRAY(*usbs, *num + 1);
> +usb = *usbs + *num;
> +(*num)++;
> +libxl_device_usb_init(usb);
> +usb->ctrl = usbctrl;
> +usb->port = i + 1;
> +rc = usb_busaddr_from_busid(gc, busid,
> +   

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

[George Dunlap]

On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu  wrote:
>> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>> > +   libxl_device_usbctrl *usbctrl,
>> > +   libxl__ao_device *aodev)
>> > +{
>>
>> Thanks for adjusting the error-handling patterns in these functions.
>> The new way is good, except that:
>>
>> > +out:
>> > +aodev->rc = rc;
>> > +if (rc) aodev->callback(egc, aodev);
>>
>> Here, rc is always set, and indeed the code would be wrong if it were
>> not.  So can you remove the conditional please ?  Ie:
>
> Reading the codes, libxl__wait_device_connection will call aodev->callback
> properly. So here, only if (rc != 0), that means error happens, then we need 
> to
> call aodev->callback to end the process. (Refer to current 
> libxl__device_disk_add,
> all current code does similar work.) So I think the code is not wrong (?)

>> > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,
>> > +  uint8_t *bus, uint8_t *addr)
>> > +{
>> > +char *filename;
>> > +void *buf;
>> > +
>> > +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);
>> > +if (!libxl__read_sysfs_file_contents(gc, filename, , NULL))
>> > +*bus = atoi((char *)buf);
>>
>> I don't think this cast (and the one for addr) are necessary ?
>
> Which cast? Here, we want to get a uint_8 value, but buf is a string,
> we need to do atoi.

atoi() isn't a cast, it's a function call.  The cast is the (char *) bit.

I think probably you could get away with declaring buf as (char *).
 should be converted to void** automatically without any warnings,
I think.

>> > +/* Encode usb interface so that it could be written to xenstore as a key.
>> > + *
>> > + * Since xenstore key cannot include '.' or ':', we'll change '.' to '_',
>> > + * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1.
>> > + * This will be used to save original driver of USB device to xenstore.
>> > + */
>>
>> What is the syntax of the incoming busid ?  Could it contain _ or - ?
>> You should perhaps spot them and reject if it does.
>
> The busid is in syntax like 3-1:2.1. It does contain '-'. But since we only 
> use
> the single direction, never decode back, so it won't harm.

So the only potential problem is that 3-1:2, 3-1-2, 3:1-2, and 3:1:2
all collapse down to 3-1-2.  Is there any risk of something like that
happening?  (Ian: NB these are all read from sysfs.)

Since this isn't really being read by anyone,  maybe it would be
better to replace ':' with another character, just to be safe.  It
could even be something like 'c' if no other punctuation is available.

>> > +/* Bind USB device to "usbback" driver.
>> > + *
>> > + * If there are many interfaces under USB device, check each interface,
>> > + * unbind from original driver and bind to "usbback" driver.
>> > + */
>> > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb)
>> > +{
>> ...
>> > +if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) 
>> > {
>> > +LOG(WARN, "Write of %s to node %s failed", drvpath, path);
>> > +}
>>
>> One of the advantages of libxl__xs_write_checked is that it logs
>> errors.  So you can leave that log message out.
>>
>> However, if the xs write fails, you should presumably stop, rather
>> than carrying on.
>>
>> 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.

Ian, I don't understand what you're saying here.  The code I'm looking at does:
1. unbind + get old driver path in drvpath
2. if(drvpath) write to xenstore
3. bind to usbback

If the bind fails, then we do have drvpath in xenstore.  Or am I
missing something?

Bailing out if the above xenstore write fails seems sensible though.

>> > +/* Operation to remove usb device.
>> > + *
>> > + * Generally, it does:
>> > + * 1) check if the usb device is assigned to the domain
>> > + * 2) remove the usb device from xenstore controller/port.
>> > + * 3) unbind usb device from usbback and rebind to its original driver.
>> > + *If usb device has many interfaces, do it to each interface.
>> > + */
>> > +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
>> > +libxl_device_usb *usb)
>> > +{
>> > +int rc;
>> > +
>> > +if (usb->ctrl < 0 || usb->port < 1) {
>> > +LOG(ERROR, "Invalid USB device");
>> > +rc = ERROR_FAIL;
>> > +goto out;
>> > +}
>> > +
>> > +if (usb->devtype == LIBXL_USBDEV_TYPE_HOSTDEV &&
>> > +(usb->u.hostdev.hostbus < 1 || usb->u.hostdev.hostaddr < 1)) {
>> > +LOG(ERROR, "Invalid USB device of hostdev");
>> > +rc = ERROR_FAIL;
>> > +goto out;
>> > +}
>>
>> Why are these checks here rather than in do_usb_remove ?
>>
>> For that 

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

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API"):
> On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu <cy...@suse.com> wrote:
> > Which cast? Here, we want to get a uint_8 value, but buf is a string,
> > we need to do atoi.
> 
> atoi() isn't a cast, it's a function call.  The cast is the (char *) bit.
> 
> I think probably you could get away with declaring buf as (char *).
>  should be converted to void** automatically without any warnings,
> I think.

No, char** won't be automatically converted to void**.  But void* will
be automatically converted to char*.

(Will read the rest of this thread later, probably tomorrow.)

Ian.

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

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

[Chun Yan Liu]

>>> On 11/10/2015 at 02:11 AM, in message
<22080.57829.461049.37...@mariner.uk.xensource.com>, Ian Jackson
 wrote: 
> Chunyan Liu writes ("[PATCH V8 3/7] 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 for this. 
>  
>  
> I have reviewed it in detail (not just the ao handling aspects) and 
> (I'm afraid) produced a large number of comments, relating to style, 
> error handling, etc., as well as ao handling. 
>  
> Please let me know if anything I've said is unclear.  I've been rather 
> brief in my comments, rather than writing a paragraph for each one. 
> Thanks. 
>  
>  
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c 
> > index dacfaae..a050e8b 100644 
> > --- a/tools/libxl/libxl.c 
> > +++ b/tools/libxl/libxl.c 
> > @@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) 
> ... 
> > +/* Macro for defining device remove/destroy functions for usbctrl */ 
> > +/* Following functions are defined: 
> > + * libxl_device_usbctrl_remove 
> > + * libxl_device_usbctrl_destroy 
> > + */ 
> > + 
> > +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)\ 
>  
> As George has said, I would prefer to avoid committing this 
> duplication in-tree, even with a promise to de-duplicated it later. 
>  
>  
> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> > +   libxl_device_usbctrl *usbctrl, 
> > +   libxl__ao_device *aodev) 
> > +{ 
>  
> Thanks for adjusting the error-handling patterns in these functions. 
> The new way is good, except that: 
>  
> > +out: 
> > +aodev->rc = rc; 
> > +if (rc) aodev->callback(egc, aodev); 
>  
> Here, rc is always set, and indeed the code would be wrong if it were 
> not.  So can you remove the conditional please ?  Ie: 

Reading the codes, libxl__wait_device_connection will call aodev->callback
properly. So here, only if (rc != 0), that means error happens, then we need to
call aodev->callback to end the process. (Refer to current 
libxl__device_disk_add,
all current code does similar work.) So I think the code is not wrong (?)

>  
>   +   aodev->callback(egc, aodev); 
>  
> The same goes for: 
>  
> > +static int 
> > +libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, libxl_device_usb  
> *usb); 
> ... 
>  
> > +/* Remove usb devices first */ 
> > +rc  = libxl__device_usb_list_for_usbctrl(gc, domid, usbctrl_devid, 
> > + , ); 
>  ^^ 
> While you're fixing other things, you could remove one of these space.s 
>  
> > +libxl_device_usbctrl * 
> > +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num) 
> > +{ 
> ... 
> > +for (usbctrl = usbctrls; usbctrl < end; 
> > + usbctrl++, entry++, (*num)++) { 
>  
> I think this would be clearer if there were a line break at the first 
> semicolon too.  Ie, I like 
> for (A; B; C) { 
> or 
> for (A; 
>  B; 
>  C) { 
>  
> But I don't like very much 
> for (A; B; 
>  C) { 
> or 
> for (A; 
>  B; C) { 
>  
> > +#define READ_SUBPATH(path, subpath) ({  \ 
> > +rc = libxl__xs_read_checked(gc, XBT_NULL,   \ 
> > +GCSPRINTF("%s/" subpath, path), \ 
> > +);  \ 
> > +if (rc) goto outerr;\ 
> > +(char *)tmp;\ 
> > +}) 
>  
> Thanks, I'm pleased with how you have done this. 
>  
> > +be_path = READ_SUBPATH(fe_path, "backend"); 
> > +usbctrl->backend_domid = atoi(READ_SUBPATH(fe_path,  
> "backend-id")); 
> > +usbctrl->version = atoi(READ_SUBPATH(be_path, "usb-ver")); 
> > +usbctrl->ports = atoi(READ_SUBPATH(be_path, "num-ports")); 
>  
> However, I have a question: 
>  
> Why do you use atoi here, but strtoul in libxl_device_usbctrl_getinfo ? 

Didn't care much about that. atoi and strtoul both work. Previously use aoti,
in libxl_device_usbctrl_getinfo, to keep consistent with other functions (like
libxl_device_disk_getinfo), use strtoul.

Will update and use the same.

>  
> > +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid, 
> > +libxl_device_usbctrl *usbctrl, 
> > +libxl_usbctrlinfo *usbctrlinfo) 
> > +{ 
> ... 
> > +tmp = READ_SUBPATH(fe_path, "backend-id"); 
> > +usbctrlinfo->backend_id = tmp ? strtoul(tmp, NULL, 10) : -1; 
>  
> There are ten copies of this pattern with tmp and strtoul.  I really 
> think this needs to be refactored somehow.  Can you 

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

[Chun Yan Liu]

>>> On 11/11/2015 at 01:57 AM, in message
, George
Dunlap  wrote: 
> On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu  wrote: 
> >> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> >> > +   libxl_device_usbctrl *usbctrl, 
> >> > +   libxl__ao_device *aodev) 
> >> > +{ 
> >> 
> >> Thanks for adjusting the error-handling patterns in these functions. 
> >> The new way is good, except that: 
> >> 
> >> > +out: 
> >> > +aodev->rc = rc; 
> >> > +if (rc) aodev->callback(egc, aodev); 
> >> 
> >> Here, rc is always set, and indeed the code would be wrong if it were 
> >> not.  So can you remove the conditional please ?  Ie: 
> > 
> > Reading the codes, libxl__wait_device_connection will call aodev->callback 
> > properly. So here, only if (rc != 0), that means error happens, then we  
> need to 
> > call aodev->callback to end the process. (Refer to current  
> libxl__device_disk_add, 
> > all current code does similar work.) So I think the code is not wrong (?) 
>  
> >> > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, 
> >> > +  uint8_t *bus, uint8_t *addr) 
> >> > +{ 
> >> > +char *filename; 
> >> > +void *buf; 
> >> > + 
> >> > +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid); 
> >> > +if (!libxl__read_sysfs_file_contents(gc, filename, , NULL)) 
> >> > +*bus = atoi((char *)buf); 
> >> 
> >> I don't think this cast (and the one for addr) are necessary ? 
> > 
> > Which cast? Here, we want to get a uint_8 value, but buf is a string, 
> > we need to do atoi. 
>  
> atoi() isn't a cast, it's a function call.  The cast is the (char *) bit. 
>  
> I think probably you could get away with declaring buf as (char *). 
>  should be converted to void** automatically without any warnings, 
> I think. 

It will report warning if declaring char *buf.

>  
> >> > +/* Encode usb interface so that it could be written to xenstore as a 
> >> > key. 
> >> > + * 
> >> > + * Since xenstore key cannot include '.' or ':', we'll change '.' to 
> >> > '_', 
> >> > + * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1. 
> >> > + * This will be used to save original driver of USB device to xenstore. 
> >> > + */ 
> >> 
> >> What is the syntax of the incoming busid ?  Could it contain _ or - ? 
> >> You should perhaps spot them and reject if it does. 
> > 
> > The busid is in syntax like 3-1:2.1. It does contain '-'. But since we only 
> >  
> use 
> > the single direction, never decode back, so it won't harm. 
>  
> So the only potential problem is that 3-1:2, 3-1-2, 3:1-2, and 3:1:2 
> all collapse down to 3-1-2.  Is there any risk of something like that 
> happening?  (Ian: NB these are all read from sysfs.) 
>  
> Since this isn't really being read by anyone,  maybe it would be 
> better to replace ':' with another character, just to be safe.  It 
> could even be something like 'c' if no other punctuation is available.

OK. Will update.
 
>  
> >> > +/* Bind USB device to "usbback" driver. 
> >> > + * 
> >> > + * If there are many interfaces under USB device, check each interface, 
> >> > + * unbind from original driver and bind to "usbback" driver. 
> >> > + */ 
> >> > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb) 
> >> > +{ 
> >> ... 
> >> > +if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 
> >> > 0)  
> { 
> >> > +LOG(WARN, "Write of %s to node %s failed", drvpath,  
> path); 
> >> > +} 
> >> 
> >> One of the advantages of libxl__xs_write_checked is that it logs 
> >> errors.  So you can leave that log message out. 
> >> 
> >> However, if the xs write fails, you should presumably stop, rather 
> >> than carrying on. 
> >> 
> >> 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. 
>  
> Ian, I don't understand what you're saying here.  The code I'm looking at  
> does: 
> 1. unbind + get old driver path in drvpath 
> 2. if(drvpath) write to xenstore 
> 3. bind to usbback 
>  
> If the bind fails, then we do have drvpath in xenstore.  Or am I 
> missing something? 

If bind to usbback fails, it will goto 'out_rebind', that is we'll try to
rebind it to its original driver and remove xenstore info. 

>  
> Bailing out if the above xenstore write fails seems sensible though. 
>  
> >> > +/* Operation to remove usb device. 
> >> > + * 
> >> > + * Generally, it does: 
> >> > + * 1) check if the usb device is assigned to the domain 
> >> > + * 2) remove the usb device from xenstore controller/port. 
> >> > + * 3) unbind usb device from usbback and rebind to its original driver. 
> >> > + *If usb device has many interfaces, do it to 

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

[Chun Yan Liu]

>>> On 11/11/2015 at 02:11 AM, in message
<22082.13115.276320.572...@mariner.uk.xensource.com>, Ian Jackson
<ian.jack...@eu.citrix.com> wrote: 
> George Dunlap writes ("Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API"): 
> > On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu <cy...@suse.com> wrote: 
> > > Which cast? Here, we want to get a uint_8 value, but buf is a string, 
> > > we need to do atoi. 
> >  
> > atoi() isn't a cast, it's a function call.  The cast is the (char *) bit. 
> >  
> > I think probably you could get away with declaring buf as (char *). 
> >  should be converted to void** automatically without any warnings, 
> > I think. 
>  
> No, char** won't be automatically converted to void**.  But void* will 
> be automatically converted to char*. 

Yes, that's right.

>  
> (Will read the rest of this thread later, probably tomorrow.) 
>  
> Ian. 
>  
>  



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

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

[Ian Jackson]

Chunyan Liu writes ("[PATCH V8 3/7] 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 for this.


I have reviewed it in detail (not just the ao handling aspects) and
(I'm afraid) produced a large number of comments, relating to style,
error handling, etc., as well as ao handling.

Please let me know if anything I've said is unclear.  I've been rather
brief in my comments, rather than writing a paragraph for each one.
Thanks.


> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index dacfaae..a050e8b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
...
> +/* Macro for defining device remove/destroy functions for usbctrl */
> +/* Following functions are defined:
> + * libxl_device_usbctrl_remove
> + * libxl_device_usbctrl_destroy
> + */
> +
> +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)\

As George has said, I would prefer to avoid committing this
duplication in-tree, even with a promise to de-duplicated it later.


> +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
> +   libxl_device_usbctrl *usbctrl,
> +   libxl__ao_device *aodev)
> +{

Thanks for adjusting the error-handling patterns in these functions.
The new way is good, except that:

> +out:
> +aodev->rc = rc;
> +if (rc) aodev->callback(egc, aodev);

Here, rc is always set, and indeed the code would be wrong if it were
not.  So can you remove the conditional please ?  Ie:

  +   aodev->callback(egc, aodev);

The same goes for:

> +static int
> +libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, libxl_device_usb 
> *usb);
...

> +/* Remove usb devices first */
> +rc  = libxl__device_usb_list_for_usbctrl(gc, domid, usbctrl_devid,
> + , );
 ^^
While you're fixing other things, you could remove one of these space.s

> +libxl_device_usbctrl *
> +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> +{
...
> +for (usbctrl = usbctrls; usbctrl < end;
> + usbctrl++, entry++, (*num)++) {

I think this would be clearer if there were a line break at the first
semicolon too.  Ie, I like
for (A; B; C) {
or
for (A;
 B;
 C) {

But I don't like very much
for (A; B;
 C) {
or
for (A;
 B; C) {

> +#define READ_SUBPATH(path, subpath) ({  \
> +rc = libxl__xs_read_checked(gc, XBT_NULL,   \
> +GCSPRINTF("%s/" subpath, path), \
> +);  \
> +if (rc) goto outerr;\
> +(char *)tmp;\
> +})

Thanks, I'm pleased with how you have done this.

> +be_path = READ_SUBPATH(fe_path, "backend");
> +usbctrl->backend_domid = atoi(READ_SUBPATH(fe_path, 
> "backend-id"));
> +usbctrl->version = atoi(READ_SUBPATH(be_path, "usb-ver"));
> +usbctrl->ports = atoi(READ_SUBPATH(be_path, "num-ports"));

However, I have a question:

Why do you use atoi here, but strtoul in libxl_device_usbctrl_getinfo ?

> +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> +libxl_device_usbctrl *usbctrl,
> +libxl_usbctrlinfo *usbctrlinfo)
> +{
...
> +tmp = READ_SUBPATH(fe_path, "backend-id");
> +usbctrlinfo->backend_id = tmp ? strtoul(tmp, NULL, 10) : -1;

There are ten copies of this pattern with tmp and strtoul.  I really
think this needs to be refactored somehow.  Can you please make a
macro which returns the return value from strtoul ?

> +static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
> +{
...
> +while ((de = readdir(dir))) {

You need to use readdir_r, I'm afraid.  Ordinary readdir is not
threadsafe.

> +}
> +
> +closedir(dir);

This assumes that all errors from readdir are EOF.  But that's not the
case.  Luckily readdir_r has more sensible error behaviour.  But you
do need to check separately for errors and EOF.

All in all this probably means that it would be clearer to write this
as:
 for (;;) {
 r = readdir_r etc.
 if (r) 
 if (!de) break;

There is at least one more call to readdir in this patch which also
needs to be fixed.

> +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,
> +  uint8_t *bus, uint8_t *addr)
> +{
> +char *filename;
> +void *buf;
> +
> +filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);
> +if (!libxl__read_sysfs_file_contents(gc, filename, 

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

[George Dunlap]

On Wed, Nov 4, 2015 at 6:31 AM, Chun Yan Liu  wrote:
> Ian & George, any comments?

Hey Chunyan,

I did actually spend a chunk of time looking at this last week.
Looking at the diff-of-diffs, it looks like you've addressed
everything I asked you to address.  I still want to take a longer look
at it before giving it a reviewed-by.  Unfortunately this will have to
wait until next week.

One thing that came up though in an offline discussion between IanJ
and I was that we would like you to actually address the
DEFINE_DEVICE_REMOVE_EXT code duplication issue before this is checked
in.  Let me know if you understand the request clearly; I'd be willing
to send you a patch you can fold in if that would be helpful.

IanJ said he has some more comments on the AO stuff as well.

 -George

>
 On 10/21/2015 at 05:08 PM, in message
> <1445418510-19614-4-git-send-email-cy...@suse.com>, Chunyan Liu
>  wrote:
>> 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
>>
>> Signed-off-by: Chunyan Liu 
>> Signed-off-by: Simon Cao 
>>
>> ---
>> changes:
>>   - update COMPARE_USB to compare ctrl and port
>>   - add check in usb_add/remove to disable non-Dom0 backend so that
>> not worring about codes which are effective on Dom0 but not
>> compatible on non-Dom0 backend.
>>   - define READ_SUBPATH macro within functions
>>   - do not initialize rc but give it value in each return case
>>   - libxl__strdup gc or NOGC update, internal function using gc,
>> external using NOGC.
>>   - address other comments from George and Ian J.
>>
>>  tools/libxl/Makefile |2 +-
>>  tools/libxl/libxl.c  |   53 ++
>>  tools/libxl/libxl.h  |   74 ++
>>  tools/libxl/libxl_device.c   |5 +-
>>  tools/libxl/libxl_internal.h |   18 +
>>  tools/libxl/libxl_osdeps.h   |   13 +
>>  tools/libxl/libxl_pvusb.c| 1451
>> ++
>>  tools/libxl/libxl_types.idl  |   57 ++
>>  tools/libxl/libxl_types_internal.idl |1 +
>>  tools/libxl/libxl_utils.c|   16 +
>>  tools/libxl/libxl_utils.h|5 +
>>  11 files changed, 1693 insertions(+), 2 deletions(-)
>>  create mode 100644 tools/libxl/libxl_pvusb.c
>>
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index c5ecec1..ef9ccd3 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -103,7 +103,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o
>> libxl_dm.o libxl_pci.o \
>>   libxl_stream_read.o libxl_stream_write.o \
>>   libxl_save_callout.o _libxl_save_msgs_callout.o \
>>   libxl_qmp.o libxl_event.o libxl_fork.o \
>> - libxl_dom_suspend.o $(LIBXL_OBJS-y)
>> + libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y)
>>  LIBXL_OBJS += libxl_genid.o
>>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index dacfaae..a050e8b 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>>
>>
>> /
>> **/
>>
>> +/* Macro for defining device remove/destroy functions for usbctrl */
>> +/* Follo:wing functions are defined:
>> + * libxl_device_usbctrl_remove
>> + * libxl_device_usbctrl_destroy
>> + */
>> +
>> +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)\
>> +int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
>> +uint32_t domid, libxl_device_##type *type,  \
>> +const libxl_asyncop_how *ao_how)\
>> +{   \
>> +AO_CREATE(ctx, domid, ao_how);  \
>> +libxl__device *device;  \
>> +libxl__ao_device *aodev;\
>> +int rc; \
>> +\
>> +GCNEW(device);  \
>> +rc = libxl__device_from_##type(gc, domid, type, device);\
>> +if (rc != 0) goto out;  \
>> +\
>> +GCNEW(aodev);   \
>> +libxl__prepare_ao_device(ao, aodev);\
>> +aodev->action = LIBXL__DEVICE_ACTION_REMOVE;\

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

[Chun Yan Liu]

Ian & George, any comments?

>>> On 10/21/2015 at 05:08 PM, in message
<1445418510-19614-4-git-send-email-cy...@suse.com>, Chunyan Liu
 wrote: 
> 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 
>  
> Signed-off-by: Chunyan Liu  
> Signed-off-by: Simon Cao  
>  
> --- 
> changes: 
>   - update COMPARE_USB to compare ctrl and port 
>   - add check in usb_add/remove to disable non-Dom0 backend so that 
> not worring about codes which are effective on Dom0 but not 
> compatible on non-Dom0 backend. 
>   - define READ_SUBPATH macro within functions 
>   - do not initialize rc but give it value in each return case 
>   - libxl__strdup gc or NOGC update, internal function using gc, 
> external using NOGC. 
>   - address other comments from George and Ian J. 
>  
>  tools/libxl/Makefile |2 +- 
>  tools/libxl/libxl.c  |   53 ++ 
>  tools/libxl/libxl.h  |   74 ++ 
>  tools/libxl/libxl_device.c   |5 +- 
>  tools/libxl/libxl_internal.h |   18 + 
>  tools/libxl/libxl_osdeps.h   |   13 + 
>  tools/libxl/libxl_pvusb.c| 1451  
> ++ 
>  tools/libxl/libxl_types.idl  |   57 ++ 
>  tools/libxl/libxl_types_internal.idl |1 + 
>  tools/libxl/libxl_utils.c|   16 + 
>  tools/libxl/libxl_utils.h|5 + 
>  11 files changed, 1693 insertions(+), 2 deletions(-) 
>  create mode 100644 tools/libxl/libxl_pvusb.c 
>  
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile 
> index c5ecec1..ef9ccd3 100644 
> --- a/tools/libxl/Makefile 
> +++ b/tools/libxl/Makefile 
> @@ -103,7 +103,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o  
> libxl_dm.o libxl_pci.o \ 
>   libxl_stream_read.o libxl_stream_write.o \ 
>   libxl_save_callout.o _libxl_save_msgs_callout.o \ 
>   libxl_qmp.o libxl_event.o libxl_fork.o \ 
> - libxl_dom_suspend.o $(LIBXL_OBJS-y) 
> + libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y) 
>  LIBXL_OBJS += libxl_genid.o 
>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o 
>   
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c 
> index dacfaae..a050e8b 100644 
> --- a/tools/libxl/libxl.c 
> +++ b/tools/libxl/libxl.c 
> @@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) 
>   
>   
> / 
> **/ 
>   
> +/* Macro for defining device remove/destroy functions for usbctrl */ 
> +/* Follo:wing functions are defined: 
> + * libxl_device_usbctrl_remove 
> + * libxl_device_usbctrl_destroy 
> + */ 
> + 
> +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)\ 
> +int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \ 
> +uint32_t domid, libxl_device_##type *type,  \ 
> +const libxl_asyncop_how *ao_how)\ 
> +{   \ 
> +AO_CREATE(ctx, domid, ao_how);  \ 
> +libxl__device *device;  \ 
> +libxl__ao_device *aodev;\ 
> +int rc; \ 
> +\ 
> +GCNEW(device);  \ 
> +rc = libxl__device_from_##type(gc, domid, type, device);\ 
> +if (rc != 0) goto out;  \ 
> +\ 
> +GCNEW(aodev);   \ 
> +libxl__prepare_ao_device(ao, aodev);\ 
> +aodev->action = LIBXL__DEVICE_ACTION_REMOVE;\ 
> +aodev->dev = device;\ 
> +aodev->callback = device_addrm_aocomplete;  \ 
> +aodev->force = f;   \ 
> +libxl__initiate_device_##type##_remove(egc, aodev); \ 
> +\ 
> +out:\ 
> +if (rc) return AO_CREATE_FAIL(rc);  \ 
> +return AO_INPROGRESS;   \ 
> +} 
> + 
> + 
> +DEFINE_DEVICE_REMOVE_EXT(usbctrl, remove, 0) 
> +DEFINE_DEVICE_REMOVE_EXT(usbctrl, destroy, 1) 
> + 
> +#undef 

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

[Juergen Gross]

On 10/21/2015 11:08 AM, Chunyan Liu wrote:

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

Signed-off-by: Chunyan Liu 
Signed-off-by: Simon Cao 

---
changes:
   - update COMPARE_USB to compare ctrl and port
   - add check in usb_add/remove to disable non-Dom0 backend so that
 not worring about codes which are effective on Dom0 but not
 compatible on non-Dom0 backend.
   - define READ_SUBPATH macro within functions
   - do not initialize rc but give it value in each return case
   - libxl__strdup gc or NOGC update, internal function using gc,
 external using NOGC.
   - address other comments from George and Ian J.

  tools/libxl/Makefile |2 +-
  tools/libxl/libxl.c  |   53 ++
  tools/libxl/libxl.h  |   74 ++
  tools/libxl/libxl_device.c   |5 +-
  tools/libxl/libxl_internal.h |   18 +
  tools/libxl/libxl_osdeps.h   |   13 +
  tools/libxl/libxl_pvusb.c| 1451 ++
  tools/libxl/libxl_types.idl  |   57 ++
  tools/libxl/libxl_types_internal.idl |1 +
  tools/libxl/libxl_utils.c|   16 +
  tools/libxl/libxl_utils.h|5 +
  11 files changed, 1693 insertions(+), 2 deletions(-)
  create mode 100644 tools/libxl/libxl_pvusb.c


...


diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
new file mode 100644
index 000..aa1a653
--- /dev/null
+++ b/tools/libxl/libxl_pvusb.c
@@ -0,0 +1,1451 @@
+/*
+ * Copyright (C) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.


Hmm, shouldn't this be just "SUSE LINUX GmbH, ..."?


+ * Author Chunyan Liu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */


...


+/* Encode usb interface so that it could be written to xenstore as a key.
+ *
+ * Since xenstore key cannot include '.' or ':', we'll change '.' to '_',
+ * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1.
+ * This will be used to save original driver of USB device to xenstore.
+ */
+static char *usb_interface_xenstore_encode(char *busid)
+{
+char *str = strdup(busid);
+int i, len = strlen(str);
+
+for (i = 0; i < len; i++) {
+if (str[i] == '.')
+str[i] = '_';
+ if (str[i] == ':')


Indentation.


+str[i] = '-';
+}
+return str;
+}


...


diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 9c5c4d0..706a0c1 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1270,6 +1270,22 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, 
size_t len)
  return ret;
  }

+void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr)
+{
+   int i;


Blank line.


+   for (i = 0; i < nr; i++)
+   libxl_device_usbctrl_dispose([i]);
+   free(list);
+}
+
+void libxl_device_usb_list_free(libxl_device_usb *list, int nr)
+{
+   int i;


Blank line.


+   for (i = 0; i < nr; i++)
+   libxl_device_usb_dispose([i]);
+   free(list);
+}



Juergen


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

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

[Chunyan Liu]

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

Signed-off-by: Chunyan Liu 
Signed-off-by: Simon Cao 

---
changes:
  - update COMPARE_USB to compare ctrl and port
  - add check in usb_add/remove to disable non-Dom0 backend so that
not worring about codes which are effective on Dom0 but not
compatible on non-Dom0 backend.
  - define READ_SUBPATH macro within functions
  - do not initialize rc but give it value in each return case
  - libxl__strdup gc or NOGC update, internal function using gc,
external using NOGC.
  - address other comments from George and Ian J.

 tools/libxl/Makefile |2 +-
 tools/libxl/libxl.c  |   53 ++
 tools/libxl/libxl.h  |   74 ++
 tools/libxl/libxl_device.c   |5 +-
 tools/libxl/libxl_internal.h |   18 +
 tools/libxl/libxl_osdeps.h   |   13 +
 tools/libxl/libxl_pvusb.c| 1451 ++
 tools/libxl/libxl_types.idl  |   57 ++
 tools/libxl/libxl_types_internal.idl |1 +
 tools/libxl/libxl_utils.c|   16 +
 tools/libxl/libxl_utils.h|5 +
 11 files changed, 1693 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxl_pvusb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index c5ecec1..ef9ccd3 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -103,7 +103,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
libxl_pci.o \
libxl_stream_read.o libxl_stream_write.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
libxl_qmp.o libxl_event.o libxl_fork.o \
-   libxl_dom_suspend.o $(LIBXL_OBJS-y)
+   libxl_dom_suspend.o libxl_pvusb.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index dacfaae..a050e8b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
 
/**/
 
+/* Macro for defining device remove/destroy functions for usbctrl */
+/* Following functions are defined:
+ * libxl_device_usbctrl_remove
+ * libxl_device_usbctrl_destroy
+ */
+
+#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)\
+int libxl_device_##type##_##removedestroy(libxl_ctx *ctx,   \
+uint32_t domid, libxl_device_##type *type,  \
+const libxl_asyncop_how *ao_how)\
+{   \
+AO_CREATE(ctx, domid, ao_how);  \
+libxl__device *device;  \
+libxl__ao_device *aodev;\
+int rc; \
+\
+GCNEW(device);  \
+rc = libxl__device_from_##type(gc, domid, type, device);\
+if (rc != 0) goto out;  \
+\
+GCNEW(aodev);   \
+libxl__prepare_ao_device(ao, aodev);\
+aodev->action = LIBXL__DEVICE_ACTION_REMOVE;\
+aodev->dev = device;\
+aodev->callback = device_addrm_aocomplete;  \
+aodev->force = f;   \
+libxl__initiate_device_##type##_remove(egc, aodev); \
+\
+out:\
+if (rc) return AO_CREATE_FAIL(rc);  \
+return AO_INPROGRESS;   \
+}
+
+
+DEFINE_DEVICE_REMOVE_EXT(usbctrl, remove, 0)
+DEFINE_DEVICE_REMOVE_EXT(usbctrl, destroy, 1)
+
+#undef DEFINE_DEVICE_REMOVE_EXT
+
+/**/
+
 /* Macro for defining device addition functions in a compact way */
 /* The following functions are defined:
  * libxl_device_disk_add
  * libxl_device_nic_add
  * libxl_device_vtpm_add
+ * libxl_device_usbctrl_add
+ * libxl_device_usb_add
  */
 
 #define DEFINE_DEVICE_ADD(type) \
@@ -4254,6 +4297,12 @@