[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v7 2/2] libxl: spice usbredirection support for upstream qemu
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.
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?
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
|
