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:44 +0000, George Dunlap wrote:
> On 12/05/2013 02:32 PM, Ian Campbell wrote:
> > 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.
> 
> I'm pretty sure Fabio had a number of back-and-forths on the qemu list 
> before formulating these patches.  Paulo Bonzini also responded to I 
> think it was Ian Jackson's question about the stability of the 
> command-line arguments, saying that the command-line arguments were a 
> stable interface that would be supported in a backwards-compatible 
> manner.  And Stefano also replied saying the qemu arguments look OK.  So 
> those have gotten about as much attention as I think they ever would.

Well, it would be nice then if that were made apparent in the changelog,
perhaps in the form of an acked-by or even just mentioning it in the
message, the fact that there was a lot of back and forth about it just
makes that more necessary.

> Re having an enum -- would it be possible in theory to switch to an enum 
> at some point in the future, in a backwards-compatible manner? I.e., 
> make the enum values for 1, 2, and 3 act just as numerical values 1, 2, 
> and 3 do now?

int -> enum should be possible, I think, unless there is some oddity
about C type conversions which I'm forgetting.

Of course people who using LIBXL_API_VERSION <= 4.4 would not be able to
use the enum names in their code, so we would have to be careful not to
change them (which for a regular enum which was never an int we might
think we can safely do).

> Overall I'm still inclined to say that the "interface risk" is fairly 
> low.  If you think differently, we can let it wait until 4.5; otherwise 
> I think we should check it in.
> 
>   -George
> 



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