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