From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v7 2/2] libxl: spice usbredirection support for upstream qemu Date: Thu, 5 Dec 2013 14:44:24 +0000 Message-ID: <52A09148.30502@eu.citrix.com> References: <1384874420-5368-1-git-send-email-fabio.fantoni@m2r.biz> <1384874420-5368-2-git-send-email-fabio.fantoni@m2r.biz> <21150.4416.91921.238457@mariner.uk.xensource.com> <1386253954.20047.69.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1386253954.20047.69.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Anthony PERARD , Fabio Fantoni , Ian Jackson , "xen-devel@lists.xensource.com" , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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 wrote: >>> Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice usbredirection support for upstream qemu"): >>>> Usage: spiceusbredirection=NUMBER (default=0) >>> ... >>>> +=item B >>>> + >>>> +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 >>> >>> 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