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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

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