On Thu, Sep 27, 2018 at 12:12 PM Christophe Fergeau <cferg...@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
>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
