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

[Christophe Fergeau]

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.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180927/666d9510/attachment.sig>