[Spice-devel] [spice-gtk v1 2/2] usb-redirection: use usb backend for usb redirection

[Yuri Benditovich]

On Thu, Sep 27, 2018 at 12:12 PM Christophe Fergeau <cfergeau at redhat.com>
wrote:

> On Thu, Sep 27, 2018 at 11:48:13AM +0300, Yuri Benditovich wrote:
> > > > -static int usbredir_read_callback(void *user_data, uint8_t *data,
> int
> > > count)
> > > > +static void usbredir_log(void *user_data, const char *msg, gboolean
> > > error)
> > > >  {
> > > >      SpiceUsbredirChannel *channel = user_data;
> > > >      SpiceUsbredirChannelPrivate *priv = channel->priv;
> > > >
> > > > -    count = MIN(priv->read_buf_size, count);
> > > > -
> > > > -    if (count != 0) {
> > > > -        memcpy(data, priv->read_buf, count);
> > > > +    if (priv->catch_error && error) {
> > > > +        g_set_error_literal(priv->catch_error, SPICE_CLIENT_ERROR,
> > > > +            SPICE_CLIENT_ERROR_FAILED, msg);
> > > > +        priv->catch_error = NULL;
> > >
> > > It's odd to set priv->catch_error and set it to NULL right after
> setting
> > > it.
> > >
> >
> > It's correct to set it to NULL if used once.
> > I'll add comment for that.
>
> If you mean that we don't want to try to set priv->catch_error multiple
> times, it probably would be clearer if you explicitly check for that:
> if (error && !error_is_set(priv->catch_error)) {
>     g_set_error_literal(...);
> }
>
> with
> bool error_is_set(GError **error)
> {
>     return ((error != NULL) && *error != NULL));
> }
>
> > > >  /**
> > > >   * spice_usb_device_get_libusb_device:
> > > > - * @device: #SpiceUsbDevice to get the descriptor information of
> > > > + * @device: #SpiceUsbDevice to get the libusb device of (if exists)
> > > >   *
> > > >   * Finds the %libusb_device associated with the @device.
> > > >   *
> > > > - * Returns: (transfer none): the %libusb_device associated to
> > > %SpiceUsbDevice.
> > > > - *
> > > > + * Returns: (transfer none): the %libusb_device associated to
> > > %SpiceUsbDevice
> > > > + *    or NULL (if the device does not have associated libusb device)
> > >
> > > This is an exported function, and if we start returning NULL in some
> > > cases, this is going to break applications using this API :(
> > >
> > >
> > This means we'll need to send commit to gnome-boxes to check returned
> value.
> > In general, when the external application (like gnome-boxes) uses
> spice-gtk
> > and does not create devices that do not have libusb_device, it never
> > find ones.
> > Are there other uses of spice-gtk except of gnome-boxes?
>
> If when you upgrade spice-gtk to a newer version, already installed apps
> which are using spice-gtk start crashing, then I'd call this an ABI
> break, which we want to avoid.. virt-viewer/remote-viewer is another
> user, virt-manager too.
>

They do not, as remote-viewer and virt-manager do not use this API.
gnome-boxes does and does not check for zero, but there is no way to create
the device without libusb with gnome-boxes.
2 questions:
1. What is the scenario where crash may happen?
2. What you suggest?


>
> Christophe
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180927/63dfd6cf/attachment.html>