Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Olaf Hering]

On Fri, Feb 19, Olaf Hering wrote:

> Not sure how to handle it, perhaps exit when xl -N is called?

Also the interface is 'xl -N' is not clearly defined. What is it
supposed to do with the newly introduced ctrl types? Should it display
the json just for the dev, just for the ctrl+dev or the entire ctrl with
all existing devs + the new dev?
Who is the consumer of the json output?
xl(1) says just "-N  Dry run: do not actually execute the command."

I'm asking this mainly in the context of main_vscsiattach, which right
now dumps the enitre ctrl with all existing devs + the new dev.

Olaf

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Olaf Hering]

On Fri, Feb 19, Chun Yan Liu wrote:

> 
> 
> >>> On 2/17/2016 at 12:56 AM, in message <20160216165608.ga21...@gmail.com>, 
> >>> Olaf
> Hering  wrote: 
> > On Tue, Jan 19, Chunyan Liu wrote: 
> >  
> > >  #xl usbctrl-attach test_vm version=1 ports=8 
> >  
> > >  #xl usbdev-attach test_vm hostbus=1 hostaddr=2 
> >  
> > I think this does not handle the -N knob of xl. Other commands check the 
> > global dryrun_only variable. 
> 
> Just sent V14. I tried to add dryrun_only codes but finally gave up. For 
> usbdev-attach,
> since it will automatically and dynamically create usb controller in some 
> cases, it's hard
> to print dryrun-only information.

Not sure how to handle it, perhaps exit when xl -N is called?

Olaf

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Chun Yan Liu]

>>> On 2/17/2016 at 12:56 AM, in message <20160216165608.ga21...@gmail.com>, 
>>> Olaf
Hering  wrote: 
> On Tue, Jan 19, Chunyan Liu wrote: 
>  
> >  #xl usbctrl-attach test_vm version=1 ports=8 
>  
> >  #xl usbdev-attach test_vm hostbus=1 hostaddr=2 
>  
> I think this does not handle the -N knob of xl. Other commands check the 
> global dryrun_only variable. 

Just sent V14. I tried to add dryrun_only codes but finally gave up. For 
usbdev-attach,
since it will automatically and dynamically create usb controller in some 
cases, it's hard
to print dryrun-only information.

Chunyan
>  
> Olaf 
>  



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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> On Tue, Feb 16, 2016 at 05:53:18PM +, Ian Jackson wrote:
> > I don't have a strong opinion, but I would lean towards insisting that
> > on command lines each setting should be in its own argument.
> 
> We should support both IMHO. Libxlu's disk spec parser is able to do
> that with parse_disk_config_multistring.

Fine by me.  As I say I don't have a strong opinion, so we should do
what Wei wants as he does :-).

Ian.

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Wei Liu]

On Tue, Feb 16, 2016 at 05:53:18PM +, Ian Jackson wrote:
> Jim Fehlig writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> > I just noticed this is the case with network devices as well. E.g.
> > 
> > #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> > libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: 
> > Could
> > not find bridge device xenbr0
> 
> This is clearly a bug.
> 
> > main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on
> > the ',', so everything after mac=00:16:3e:xx:yy:zz is ignored.
> 
> That's quite silly, isn't it.  Looking at the code it would also accept
>   mac=00:16:3e:aa:bb:cc:THIS:SHOULD:NOT:BE:HERE
> 
> The bug seems to be that the ad-hoc strtoul-based mac address parser
> in xl_cmdimpl.c:parse_nic_config doesn't notice garbage after its
> option.
> 
> > I'd need advice on how to fix this though. Based on
> > xl-network-configuration doc and Xen tool's long history of
> > network-attach supporting that syntax, I'd say main_networkattach()
> > should be changed to split on ','. I could also change the docs. Do
> > tools maintainers have a preference, or alternative option?
> 
> I don't have a strong opinion, but I would lean towards insisting that
> on command lines each setting should be in its own argument.
> 

We should support both IMHO. Libxlu's disk spec parser is able to do
that with parse_disk_config_multistring.

Wei.

> Ian.

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Chun Yan Liu]

n

>>> On 2/17/2016 at 12:56 AM, in message <20160216165608.ga21...@gmail.com>, 
>>> Olaf
Hering  wrote: 
> On Tue, Jan 19, Chunyan Liu wrote: 
>  
> >  #xl usbctrl-attach test_vm version=1 ports=8 
>  
> >  #xl usbdev-attach test_vm hostbus=1 hostaddr=2 
>  
> I think this does not handle the -N knob of xl. Other commands check the 
> global dryrun_only variable. 

Thanks. I'll add.

>  
> 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 V13 5/5] xl: add pvusb commands

[Ian Jackson]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> I just noticed this is the case with network devices as well. E.g.
> 
> #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
> not find bridge device xenbr0

This is clearly a bug.

> main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on
> the ',', so everything after mac=00:16:3e:xx:yy:zz is ignored.

That's quite silly, isn't it.  Looking at the code it would also accept
  mac=00:16:3e:aa:bb:cc:THIS:SHOULD:NOT:BE:HERE

The bug seems to be that the ad-hoc strtoul-based mac address parser
in xl_cmdimpl.c:parse_nic_config doesn't notice garbage after its
option.

> I'd need advice on how to fix this though. Based on
> xl-network-configuration doc and Xen tool's long history of
> network-attach supporting that syntax, I'd say main_networkattach()
> should be changed to split on ','. I could also change the docs. Do
> tools maintainers have a preference, or alternative option?

I don't have a strong opinion, but I would lean towards insisting that
on command lines each setting should be in its own argument.

Ian.

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Olaf Hering]

On Tue, Jan 19, Chunyan Liu wrote:

>  #xl usbctrl-attach test_vm version=1 ports=8

>  #xl usbdev-attach test_vm hostbus=1 hostaddr=2

I think this does not handle the -N knob of xl. Other commands check the
global dryrun_only variable.

Olaf

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Ian Campbell]

On Thu, 2016-01-21 at 12:12 +, Wei Liu wrote:
> On Tue, Jan 19, 2016 at 08:10:07PM -0700, Jim Fehlig wrote:
> > On 01/19/2016 11:11 AM, Ian Jackson wrote:
> > > Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> > > > Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> > > > usbdev-attach and usbdev-detach.
> > > Thanks for swapping this with the other patch.  It is better now.
> > > 
> > > > +=item B I I
> > > However, I think you need to explictly state that the user may (and
> > > indeed, must) pass multiple settings as separate arguments.  AFAICT
> > > the parser here doesn't do the ,-splitting.
> > 
> > I just noticed this is the case with network devices as well. E.g.
> > 
> > #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> > libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb:
> > script: Could
> > not find bridge device xenbr0
> > 
> > main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the
> > ',', so
> > everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on
> > how to fix
> > this though. Based on xl-network-configuration doc and Xen tool's long
> > history
> > of network-attach supporting that syntax, I'd say main_networkattach()
> > should be
> > changed to split on ','. I could also change the docs. Do tools
> > maintainers have
> > a preference, or alternative option?
> > 
> 
> It is splitting on " " at the moment which is not very desirable.
> 
> I think this is a bug. I've added it to my list and will look into
> fixing it.

If you are feeling particularly highly motivated then xl_cmdimpl.c contains
quite a lot of adhoc parsing of various comma a space separated lists
(often dupped for a given instance into cmdline vs cfg file, which leads to
unintentional behavioural differences like above) which could usefully be
consolidated into a series of helpers.

Ian.


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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Wei Liu]

On Tue, Jan 19, 2016 at 08:10:07PM -0700, Jim Fehlig wrote:
> On 01/19/2016 11:11 AM, Ian Jackson wrote:
> > Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> >> Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> >> usbdev-attach and usbdev-detach.
> > Thanks for swapping this with the other patch.  It is better now.
> >
> >> +=item B I I
> > However, I think you need to explictly state that the user may (and
> > indeed, must) pass multiple settings as separate arguments.  AFAICT
> > the parser here doesn't do the ,-splitting.
> 
> I just noticed this is the case with network devices as well. E.g.
> 
> #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
> not find bridge device xenbr0
> 
> main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the ',', so
> everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on how to 
> fix
> this though. Based on xl-network-configuration doc and Xen tool's long history
> of network-attach supporting that syntax, I'd say main_networkattach() should 
> be
> changed to split on ','. I could also change the docs. Do tools maintainers 
> have
> a preference, or alternative option?
> 

It is splitting on " " at the moment which is not very desirable.

I think this is a bug. I've added it to my list and will look into
fixing it.

Wei.

> Regards,
> Jim
> 

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Wei Liu]

On Thu, Jan 21, 2016 at 12:21:11PM +, Ian Campbell wrote:
> On Thu, 2016-01-21 at 12:12 +, Wei Liu wrote:
> > On Tue, Jan 19, 2016 at 08:10:07PM -0700, Jim Fehlig wrote:
> > > On 01/19/2016 11:11 AM, Ian Jackson wrote:
> > > > Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> > > > > Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> > > > > usbdev-attach and usbdev-detach.
> > > > Thanks for swapping this with the other patch.  It is better now.
> > > > 
> > > > > +=item B I I
> > > > However, I think you need to explictly state that the user may (and
> > > > indeed, must) pass multiple settings as separate arguments.  AFAICT
> > > > the parser here doesn't do the ,-splitting.
> > > 
> > > I just noticed this is the case with network devices as well. E.g.
> > > 
> > > #xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
> > > libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb:
> > > script: Could
> > > not find bridge device xenbr0
> > > 
> > > main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the
> > > ',', so
> > > everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on
> > > how to fix
> > > this though. Based on xl-network-configuration doc and Xen tool's long
> > > history
> > > of network-attach supporting that syntax, I'd say main_networkattach()
> > > should be
> > > changed to split on ','. I could also change the docs. Do tools
> > > maintainers have
> > > a preference, or alternative option?
> > > 
> > 
> > It is splitting on " " at the moment which is not very desirable.
> > 
> > I think this is a bug. I've added it to my list and will look into
> > fixing it.
> 
> If you are feeling particularly highly motivated then xl_cmdimpl.c contains
> quite a lot of adhoc parsing of various comma a space separated lists
> (often dupped for a given instance into cmdline vs cfg file, which leads to
> unintentional behavioural differences like above) which could usefully be
> consolidated into a series of helpers.
> 

Or rewrite every adhoc parser with bison/flex or Ocaml / Haskell parsec.

Jokes aside. I will see what I can do. Consolidating them into helper
functions is a good start.

Wei.

> Ian.
> 

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands"):
> Or rewrite every adhoc parser with bison/flex or Ocaml / Haskell parsec.

I endorse this product and/or service.

> Jokes aside. I will see what I can do. Consolidating them into helper
> functions is a good start.

Yes :-).

Ian.

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

[Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Chunyan Liu]

Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
usbdev-attach and usbdev-detach.

To attach a usb device to guest through pvusb, one could follow
following example:

 #xl usbctrl-attach test_vm version=1 ports=8

 #xl usb-list test_vm
 will show the usb controllers and port usage under the domain.

 #xl usbdev-attach test_vm hostbus=1 hostaddr=2
 will find the first usable controller:port, and attach usb
 device whose busnum is 1 and devnum is 6.
 One could also specify which  and which .

 #xl usbdev-detach test_vm 0 1
 will detach USB device under controller 0 port 1.

 #xl usbctrl-detach test_vm dev_id
 will destroy the controller with specified dev_id. Dev_id
 can be traced in usb-list info.

Signed-off-by: Chunyan Liu 
Signed-off-by: Simon Cao 
Reviewed-by: George Dunlap 
---
changes:
* update docs to reuse documentation in xl.cfg
* remove backend patch information from usb-list

 docs/man/xl.pod.1 |  35 +
 tools/libxl/xl.h  |   5 ++
 tools/libxl/xl_cmdimpl.c  | 190 ++
 tools/libxl/xl_cmdtable.c |  25 ++
 4 files changed, 255 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 4279c7c..7e0a380 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1345,6 +1345,41 @@ List pass-through pci devices for a domain.
 
 =back
 
+=head1 USB PASS-THROUGH
+
+=over 4
+
+=item B I I
+
+Create a new USB controller in the domain specified by I,
+I describes the device to attach, using the same format
+as the B string in the domain config file. See L for
+more information.
+
+=item B I I
+
+Destroy a USB controller from the specified domain.
+B is devid of the USB controller.
+
+=item B I I
+
+Hot-plug a new pass-through USB device to the domain specified by
+I, I describes the device to attach, using
+the same format as the B string in the domain config file.
+See L for more information.
+
+=item B I 

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Ian Jackson]

Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
> Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
> usbdev-attach and usbdev-detach.

Thanks for swapping this with the other patch.  It is better now.

> +=item B I I

However, I think you need to explictly state that the user may (and
indeed, must) pass multiple settings as separate arguments.  AFAICT
the parser here doesn't do the ,-splitting.

> +I describes the device to attach, using the same format
> +as the B string in the domain config file. See L for
> +more information.

And this, therefore, is not quite true.

To be clear, I think that you should fix the documentation to match
the code.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands

[Jim Fehlig]

On 01/19/2016 11:11 AM, Ian Jackson wrote:
> Chunyan Liu writes ("[PATCH V13 5/5] xl: add pvusb commands"):
>> Add pvusb commands: usbctrl-attach, usbctrl-detach, usb-list,
>> usbdev-attach and usbdev-detach.
> Thanks for swapping this with the other patch.  It is better now.
>
>> +=item B I I
> However, I think you need to explictly state that the user may (and
> indeed, must) pass multiple settings as separate arguments.  AFAICT
> the parser here doesn't do the ,-splitting.

I just noticed this is the case with network devices as well. E.g.

#xl network-attach hvm-domU mac=00:16:3e:xx:yy:zz,bridge=br0
libxl: error: libxl_device.c:1095:device_hotplug_child_death_cb: script: Could
not find bridge device xenbr0

main_networkattach() in tools/libxl/xl_cmdimpl.c doesn't split on the ',', so
everything after mac=00:16:3e:xx:yy:zz is ignored. I'd need advice on how to fix
this though. Based on xl-network-configuration doc and Xen tool's long history
of network-attach supporting that syntax, I'd say main_networkattach() should be
changed to split on ','. I could also change the docs. Do tools maintainers have
a preference, or alternative option?

Regards,
Jim


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