Re: [Xen-devel] [PATCH v7 2/2] libxl: spice usbredirection support for upstream qemu

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-12-05 at 14:22 +0000, George Dunlap wrote:
> On Tue, Dec 3, 2013 at 5:13 PM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> > Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice usbredirection support 
> > for upstream qemu"):
> >> Usage: spiceusbredirection=NUMBER (default=0)
> > ...
> >> +=item B<spiceusbredirection=NUMBER>
> >> +
> >> +Enables spice usbredirection. Creates NUMBER usbredirection channels
> >> +for redirection of up to 4 usb devices from spice client to domU's qemu.
> >> +It requires an usb controller and if not defined it will automatically 
> >> adds
> >> +an usb2 controller. The default is disabled (0).
> >
> > I see we have:
> >   spice
> >   spiceport
> >   spicetls_port
> >   spicehost
> >   spicedisable_ticketing
> >   spicepasswd
> >   spiceagent_mouse
> >   spicevdagent
> >   spice_clipboard_sharing
> >
> > This is a mess in terms of the spelling of the parameters.  But I
> > guess the new one is with the majority.
> >
> > The bulk of the patch seems fine.
> >
> > Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >
> > This needs a freeze exception.  George ?
> 
> Hrm.  I definitely would have given an Ack two weeks ago when this was
> first submitted.  Now it seems fairly late in the freeze.
> 
> The patches themselves are fairly simple, and have had a lot of
> review.  If there is a bug, it is very unlikely to affect users who
> are not directly using these features (which are off by default); for
> users who are using these, the worst that could happen is that the xl
> create fails, or the feature does not work.  (i.e., random guest or
> host crashes cannot be a result of a bug here, but of a bug somewhere
> else that this might trigger).
> 
> I guess the big risk here is the interface.  This series only
> introduces two new elements: "usbversion", and "spice.usbredirection".
>  Both of those seem like things we would want to introduce, and given
> that, it's hard to imagine what other interface we would even use.
> We've had a lot of review cycles for the interface, so I think that is
> overall fairly low risk.

My only question would be should usbversion be an enum rather than a
numeric version, especially since USB1 had two main controller types
(ohci and uhci). USB 2+ seems to be pretty standard with ehci and xhci.
An enum is also more self documenting. As a side effect it offers you a
parsing function so you can use descriptive text in the cfg file too.

On the other hand maybe users are going to be happier with usb1/2/3 than
with uhci/ehci/xhci (TBH I had to look up which was which...)

Has a qemu person acked the command lines used to create each of these
configurations? To my non-usb aware mind it looks odd that USB2
explicitly creates the legacy USB1 controller (AIUI this is how USB2 is
supposed to be done) while USB3 doesn't create any legacy controllers. I
don't know how USB3 is suppose to work, so perhaps USB3 does differ from
USB2 in this way, I don't know.

Ian.

> 
> I'm inclined therefore to give this a release Ack.  Any other opinions?
> 
>  -George



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