[Spice-devel] [PATCH 2/9] add spice_usb_device_manager shared CD related api functions
Yuri Benditovich
yuri.benditovich at daynix.com
Fri Dec 6 11:38:14 UTC 2019
On Fri, Dec 6, 2019 at 12:04 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > From: Alexander Nezhinsky <anezhins at redhat.com>
> >
> > The following functions are added:
> > spice_usb_device_manager_create_shared_cd_device
> > spice_usb_device_manager_is_device_shared_cd
> > spice_usb_device_manager_remove_shared_cd_device
> >
> > Signed-off-by: Alexander Nezhinsky <anezhins at redhat.com>
> > ---
> > src/map-file | 3 ++
> > src/usb-device-manager.c | 89 ++++++++++++++++++++++++++++++++++++++++
> > src/usb-device-manager.h | 14 +++++++
> > 3 files changed, 106 insertions(+)
> >
> > diff --git a/src/map-file b/src/map-file
> > index 3cb9873..5ae56c3 100644
> > --- a/src/map-file
> > +++ b/src/map-file
> > @@ -180,6 +180,9 @@ spice_usb_device_manager_get_devices_with_filter;
> > spice_usb_device_manager_get_type;
> > spice_usb_device_manager_is_device_connected;
> > spice_usb_device_manager_is_redirecting;
> > +spice_usb_device_manager_create_shared_cd_device;
> > +spice_usb_device_manager_remove_shared_cd_device;
> > +spice_usb_device_manager_is_device_shared_cd;
> > spice_usb_device_widget_get_type;
> > spice_usb_device_widget_new;
> > spice_usbredir_channel_get_type;
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > index 3a9542a..0961ef9 100644
> > --- a/src/usb-device-manager.c
> > +++ b/src/usb-device-manager.c
> > @@ -32,6 +32,7 @@
> > #endif
> >
> > #include "channel-usbredir-priv.h"
> > +#include "usb-device-cd.h"
> > #endif
> >
> > #include "spice-session-priv.h"
> > @@ -1439,7 +1440,94 @@ gchar *spice_usb_device_get_description(SpiceUsbDevice
> > *device, const gchar *for
> > #endif
> > }
> >
> > +/**
> > + * spice_usb_device_manager_create_shared_cd_device:
> > + * @self: a #SpiceUsbDeviceManager
> > + * @filename: image or device path
> > + * @err: (allow-none): a return location for a #GError, or %NULL.
> > + *
> > + * Creates a new shared CD device based on a disk image file
> > + * or a physical CD device.
> > + *
> > + * Returns: %TRUE if device created successfully
> > + */
> > +gboolean
> > +spice_usb_device_manager_create_shared_cd_device(
> > + SpiceUsbDeviceManager *self,
> > + gchar *filename,
> > + GError **err)
>
> style, see https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html,
> here in a lot of declarations
>
> Also for consistency in the other file "manager" is used, not "self".
>
> It's weird that the create does not return the device so you cannot
> easily use spice_usb_device_manager_remove_shared_cd_device to remove it.
This is by design and (IMO) is completely correct that the 'create'
does not return the the backend device.
Usb manager receives new devices only via hot plug indication. It
can't deal with the device until it is reported to it.
>
> > +{
> > +#ifdef USE_USBREDIR
> > + SpiceUsbDeviceManagerPrivate *priv = self->priv;
> > +
> > + CdEmulationParams cd_params = {
> > + .filename = filename,
> > + .delete_on_eject = 1,
> > + };
> > +
> > + return create_emulated_cd(priv->context, &cd_params, err);
> > +#else
> > + g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > + _("USB redirection support not compiled in"));
> > + return FALSE;
> > +#endif
> > +}
> > +
> > +/**
> > + * spice_usb_device_manager_remove_shared_cd_device:
> > + * @self: a #SpiceUsbDeviceManager
> > + * @device: a #SpiceUsbDevice to remove
> > + * @err: (allow-none): a return location for a #GError, or %NULL.
> > + *
> > + * Removes a shared CD device.
> > + *
> > + * Returns: %TRUE if device removed successfully
> > + */
> > +gboolean
> > +spice_usb_device_manager_remove_shared_cd_device(SpiceUsbDeviceManager
> > *self,
> > + SpiceUsbDevice
> > *device,
> > + GError **err)
> > +{
> > +#ifdef USE_USBREDIR
> > + SpiceUsbBackendDevice *bdev;
> >
> > + bdev = spice_usb_device_manager_device_to_bdev(self, device);
>
> see below
>
> > + spice_usb_backend_device_eject(self->priv->context, bdev);
> > + spice_usb_backend_device_unref(bdev);
> > + return TRUE;
> > +#else
> > + g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > + _("USB redirection support not compiled in"));
> > + return FALSE;
> > +#endif
> > +}
> > +
> > +/**
> > + * spice_usb_device_manager_is_device_shared_cd:
> > + * @self: a #SpiceUsbDeviceManager
> > + * @device: a #SpiceUsbDevice to query
> > + *
> > + * Checks whether a device is shared CD.
> > + *
> > + * Returns: %TRUE if the device is shared CD
> > + */
> > +gboolean
> > +spice_usb_device_manager_is_device_shared_cd(SpiceUsbDeviceManager *self,
> > + SpiceUsbDevice *device)
> > +{
> > +#ifdef USE_USBREDIR
> > + SpiceUsbBackendDevice *bdev;
> > + gboolean is_cd;
> > +
> > + bdev = spice_usb_device_manager_device_to_bdev(self, device);
>
> Note that SpiceUsbBackendDevice is defined as
>
> typedef struct _SpiceUsbDevice SpiceUsbBackendDevice;
>
> no need to call this function.
In each external API the UsbDeviceManager converts spice device to
referenced backend device
and dereferences the backend device later. This API is not different
from other ones.
IMO, in separate commit the code can be simplified in all the APIs.
>
> > + is_cd = (spice_usb_backend_device_get_libdev(bdev) == NULL) ? TRUE :
> > FALSE;
>
> just
>
> is_cd = (spice_usb_backend_device_get_libdev(bdev) == NULL);
>
> Also the name of the function is wrong, you are not checking if the device
> is a shared cd but if is a emulated device.
I think, it is correct to have external API 'is_emulated_cd' (because
we use 'Spice CD' label in the widget)
In the implementation (because currently we have only emulated cd) it
is ok to simplify the check.
If you do not agree, what you'd suggest?
>
> > + spice_usb_backend_device_unref(bdev);
> > +
> > + return is_cd;
> > +#else
> > + return FALSE;
> > +#endif
> > +}
> >
> > #ifdef USE_USBREDIR
> > /*
> > @@ -1499,6 +1587,7 @@ gboolean spice_usb_device_is_isochronous(const
> > SpiceUsbDevice *info)
> > return spice_usb_backend_device_isoch((SpiceUsbBackendDevice*) info);
> > }
> >
> > +
>
> spurious hunk, remove
>
> > #ifdef G_OS_WIN32
> > static
> > gboolean _usbdk_hider_prepare(SpiceUsbDeviceManager *manager)
> > diff --git a/src/usb-device-manager.h b/src/usb-device-manager.h
> > index 773208f..dc1a644 100644
> > --- a/src/usb-device-manager.h
> > +++ b/src/usb-device-manager.h
> > @@ -143,6 +143,20 @@
> > spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager *self,
> >
> > gboolean spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager
> > *self);
> >
> > +gboolean
> > +spice_usb_device_manager_create_shared_cd_device(
> > + SpiceUsbDeviceManager *self,
> > + gchar
> > *filename,
> > + GError **err);
> > +gboolean
> > +spice_usb_device_manager_remove_shared_cd_device(
> > + SpiceUsbDeviceManager *self,
> > + SpiceUsbDevice *device,
> > + GError **err);
> > +gboolean
> > +spice_usb_device_manager_is_device_shared_cd(SpiceUsbDeviceManager *self,
> > + SpiceUsbDevice *device);
> > +
> > G_END_DECLS
> >
> > #endif /* __SPICE_USB_DEVICE_MANAGER_H__ */
>
> Frediano
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list