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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@eu.citrix.com> 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@eu.citrix.com>
>>>
>>> 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