Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands
On Wed, Feb 24, Wei Liu wrote: > Say, if you only add a controller, you just print the ctrl JSON. If you > add a controller and a bunch of devices, you print all of them. Does > this sound plausible? Yes. 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
On Fri, Feb 19, 2016 at 01:07:08PM +0100, Olaf Hering wrote: > 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. > I just turn everything into JSON and print it out? Say, if you only add a controller, you just print the ctrl JSON. If you add a controller and a bunch of devices, you print all of them. Does this sound plausible? I tend to think dryrun output is only used by administrator to eye-ball the resulting structure. I'm not sure if it is meant to be stable at all. Maybe Ian has a second opinion on this. Wei. > 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
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
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
>>> 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
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
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
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
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
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
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
Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands
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
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
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
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
Re: [Xen-devel] [PATCH V13 5/5] xl: add pvusb commands
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
