[Spice-devel] [spice-gtk 3/9] usb-redir: Prepare for creation of emulated CD drive

[Frediano Ziglio]

> 
> On Thu, Jul 25, 2019 at 1:32 PM Victor Toso <victortoso at redhat.com> wrote:
> >
> > Hi,
> >
> > On Thu, Jul 25, 2019 at 05:09:17AM -0400, Frediano Ziglio wrote:
> > > >
> > > > Added command-line option for shared CD devices and respective
> > > > property in usb-device-manager.
> > > >
> > > > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > > > ---
> > > >  src/spice-option.c       | 15 +++++++++++++++
> > > >  src/usb-device-manager.c | 33 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 48 insertions(+)
> > > >
> > > > diff --git a/src/spice-option.c b/src/spice-option.c
> > > > index c2b059e..4fbca9f 100644
> > > > --- a/src/spice-option.c
> > > > +++ b/src/spice-option.c
> > > > @@ -32,6 +32,7 @@ static char *smartcard_db = NULL;
> > > >  static char *smartcard_certificates = NULL;
> > > >  static char *usbredir_auto_redirect_filter = NULL;
> > > >  static char *usbredir_redirect_on_connect = NULL;
> > > > +static gchar **cd_share_files = NULL;
> > > >  static gboolean smartcard = FALSE;
> > > >  static gboolean disable_audio = FALSE;
> > > >  static gboolean disable_usbredir = FALSE;
> > > > @@ -183,6 +184,8 @@ GOptionGroup* spice_get_option_group(void)
> > > >            N_("Filter selecting USB devices to be auto-redirected when
> > > >            plugged in"), N_("<filter-string>") },
> > > >          { "spice-usbredir-redirect-on-connect", '\0', 0,
> > > >          G_OPTION_ARG_STRING, &usbredir_redirect_on_connect,
> > > >            N_("Filter selecting USB devices to redirect on connect"),
> > > >            N_("<filter-string>") },
> > > > +        { "spice-share-cd", '\0', 0, G_OPTION_ARG_STRING_ARRAY,
> > > > &cd_share_files,
> > > > +          N_("Name of ISO file or CD/DVD device to share"),
> > > > N_("<filename>
> > > > (repeat allowed)") },
> > > >          { "spice-cache-size", '\0', 0, G_OPTION_ARG_INT, &cache_size,
> > > >            N_("Image cache size (deprecated)"), N_("<bytes>") },
> > > >          { "spice-glz-window-size", '\0', 0, G_OPTION_ARG_INT,
> > > >          &glz_window_size,
> > > > @@ -272,6 +275,18 @@ void spice_set_session_option(SpiceSession
> > > > *session)
> > > >              g_object_set(m, "redirect-on-connect",
> > > >                           usbredir_redirect_on_connect, NULL);
> > > >      }
> > > > +    if (cd_share_files) {
> > > > +        SpiceUsbDeviceManager *m =
> > > > spice_usb_device_manager_get(session,
> > > > NULL);
> > > > +        if (m) {
> > > > +            gchar **name = cd_share_files;
> > > > +            while (name && *name) {
> > > > +                g_object_set(m, "share-cd", *name, NULL);
> > > > +                name++;
> > > > +            }
> > > > +        }
> > > > +        g_strfreev(cd_share_files);
> > > > +        cd_share_files = NULL;
> > > > +    }
> > > >      if (disable_usbredir)
> > > >          g_object_set(session, "enable-usbredir", FALSE, NULL);
> > > >      if (disable_audio)
> > > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > > index 0961d16..b11bb15 100644
> > > > --- a/src/usb-device-manager.c
> > > > +++ b/src/usb-device-manager.c
> > > > @@ -75,6 +75,7 @@ enum {
> > > >      PROP_AUTO_CONNECT_FILTER,
> > > >      PROP_REDIRECT_ON_CONNECT,
> > > >      PROP_FREE_CHANNELS,
> > > > +    PROP_SHARE_CD
> > > >  };
> > > >
> > > >  enum
> > > > @@ -433,6 +434,26 @@ static void
> > > > spice_usb_device_manager_set_property(GObject       *gobject,
> > > >          priv->redirect_on_connect = g_strdup(filter);
> > > >          break;
> > > >      }
> > > > +    case PROP_SHARE_CD:
> > > > +    {
> > > > +#ifdef USE_USBREDIR
> > > > +        UsbCreateDeviceParameters param = { 0 };
> > > > +        const gchar *name = g_value_get_string(value);
> > > > +        /* the string is temporary, no need to keep it */
> > > > +        SPICE_DEBUG("share_cd set to %s", name);
> > > > +        if (name[0] == '!') {
> > > > +            name++;
> > > > +            param.device_param.create_cd.delete_on_eject = 1;
> > > > +        }
> > >
> > > This "!" should be documented if the interface should be public.
> > >
> > > > +        param.device_param.create_cd.filename = name;
> > > > +        if (!spice_usb_backend_create_device(priv->context,
> > > > USB_DEV_TYPE_CD,
> > > > &param)) {
> > > > +            g_warning(param.error->message);
> > > > +            spice_usb_device_manager_device_error(self, NULL,
> > > > param.error);
> > > > +            g_error_free(param.error);
> > > > +        }
> > > > +#endif
> > > > +        break;
> > > > +    }
> > >
> > > This is not a property. This is the property interface used to add a
> > > device.
> >
> > Yes, not quite sure why this is done this way. Perhaps there is a
> > reason in future patches but I'd say it is better to have some
> > public api to add/remove cd-rom instead of a reusable string
> > property that itself it doesn't hold any value in the object
> 
> Public interface is not something we can easily change in future, so I would
> not
> run to define it before there is decision about UI.
> 

Fine with it but in your commit there's no "RFC", "EXP", "TODO" or any other
indication about this change is just temporary to discuss.
If the commit goes online and somebody package it and distribute it can become
an ABI.
Maybe an option to Meson (disabled by default) to disable this feature?
Also patch should be one of the last.

> >
> > >
> > > >      default:
> > > >          G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
> > > >          break;
> > > > @@ -523,6 +544,18 @@ static void
> > > > spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
> > > >      g_object_class_install_property(gobject_class,
> > > >      PROP_REDIRECT_ON_CONNECT,
> > > >                                      pspec);
> > > >
> > > > +    /**
> > > > +    * SpiceUsbDeviceManager:share-cd:
> > > > +    *
> > > > +    * Set a string specifying a filename (ISO) or physical CD/DVD
> > > > device
> > > > +    * to share via USB after a Spice connection has been established.
> > > > +    *
> > > > +    */
> > > > +    pspec = g_param_spec_string("share-cd", "Share ISO file or device
> > > > as
> > > > CD",
> > > > +        "File or device name to share", NULL,
> > > > +        G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
> > > > +    g_object_class_install_property(gobject_class, PROP_SHARE_CD,
> > > > pspec);
> > > > +
> > > >      /**
> > > >       * SpiceUsbDeviceManager:free-channels:
> > > >       *
> > >

Frediano