[Spice-devel] [PATCH shared-cd v1 0/9] Add Spice CD functionality to usb-device-widget

[Frediano Ziglio]

> 
> From: Alexander Nezhinsky <anezhins at redhat.com>
> 
> This patch-set adds shared Spice CD functionality to usb-device-widget.
> 
> Every time the widget is created or redrawn, a placeholder toggle named
> "Spice CD (empty)" appears on top of the available USB devices list.
> It allows adding CD devices and, when clicked, fires up a file chooser.
> 
> If an image path has been selected, a new Spice USB CD device is created.
> After the new device is added and this event is signalled to the widget,
> the original toggle shows the image file path and starts representing
> the CD. A new placeholder toggle for an empty Spice CD is added.
> 
> When the media is unloaded, the shared CD device is removed. Otherwise,
> all standard scenarios for Spice USB device removal are supported.
> 
> Necessary functionality and api for shared CD maintainance have been added
> to usb-device-manager.
> 

I'm looking at the average series.

I tested the GUI and I found nice and easy. You click, provide a CD ISO image
and a new device is ready and connected.
I found however patches 2/9 and 8/9 (command line interface) a bit different,
I would expect especially from 8/9 that all devices, like the GUI, be connected,
so not filtered at all. Maybe this is the issue solved by 9/9?
Also I find 6/9 and 7/9 a bit odd. The auto-removal is done by the device
itself which is set on delete_on_close. However on these patches the same
job is done at a much higher level, in the GUI. Also the path looks like "long":
the device manager call the GUI which detect the shared CD using the device
manager and tell the device manager what to do. Would be moving all the logic
to device manager directly easier? I think would also remove the necessity
of both spice_usb_device_manager_is_device_shared_cd and
spice_usb_device_manager_remove_shared_cd_device API.

I think creating the "empty_cd" before the device manager would avoid the
check for "empty_cd != NULL" later and prevent some possible issues
(like devices not correctly connected)

I think 1/9 (especially) and 4/9 as preparatory and not specifically CD interface
related, can these be moved as first and possible be merged (can I do the job?)

These comments are not much expecting code/comments changes, more trying
to share impressions and better understanding the overall changes and
expectations.

> Alexander Nezhinsky (9):
>   move spice_usb_backend_device_{eject|report_change} to usb_backend.h
>   add spice_usb_device_manager shared CD related api functions
>   Add --spice-share-cd command line option
>   Factor out spice_usb_device_widget_add_err_msg() in usb-device-widget
>   Add empty CD entry to usb-device-widget, create shared CD when toggled
>   Auto-remove shared CD devices on disconnect in usb-device-widget
>   remove shared CD device on connect-fail during USB redirect attempt
>   auto-connect shared CD devices added using command line
>   remove unconnected shared CDs upon usb-device-widget construction
> 
>  src/map-file             |   3 +
>  src/spice-option.c       |  28 +++++++
>  src/usb-backend.h        |   2 +
>  src/usb-device-manager.c | 112 ++++++++++++++++++++++++++--
>  src/usb-device-manager.h |  14 ++++
>  src/usb-device-widget.c  | 157 +++++++++++++++++++++++++++++++++++----
>  src/usb-emulation.h      |   3 +-
>  7 files changed, 295 insertions(+), 24 deletions(-)
> 
> --
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel