[Spice-devel] [PATCH V2] usbredir: fix redirection of user-accesible device nodes.

[Michal Suchanek]

Excerpts from Christophe Fergeau's message of Fri Jul 10 13:49:28 +0200 2015:
> On Fri, Jul 10, 2015 at 01:33:41PM +0200, Michal Suchanek wrote:
> > Excerpts from Christophe Fergeau's message of Fri Jul 10 10:23:33 +0200 2015:
> > > On Fri, Jul 10, 2015 at 10:18:15AM +0200, Michal Suchanek wrote:
> > > > Excerpts from Christophe Fergeau's message of Thu Jul 09 17:11:45 +0200 2015:
> > > > > On Thu, Jul 09, 2015 at 04:58:52PM +0200, Michal Suchanek wrote:
> > > > > > Excerpts from Christophe Fergeau's message of Thu Jul 09 16:41:58 +0200 2015:
> > > > > > > > -    spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &err);
> > > > > > > > -    if (!err && priv->state == STATE_DISCONNECTING) {
> > > > > > > > +    spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &acl_err);
> > > > > > > > +    if (!acl_err && priv->state == STATE_DISCONNECTING) {
> > > > > > > 
> > > > > > > Here I'm wondering if you should not drop the if (!acl_err) part.
> > > > > > 
> > > > > > That's the original code. It's probably meant to show the cancelling
> > > > > > error only if no other error happened so far.
> > > > > > 
> > > > > > Either way if acl_err is set it will be shown regardless of err.
> > > > > 
> > > > > The initial code reads "if state is DISCONNECTING, then set 'err' if
> > > > > it's not set already" that is, "if _open_acl_finish() returned an error,
> > > > > or if state is DISCONNECTING, then 'err' is set, and we will report an
> > > > > error"
> > > > > After your changes, this has become "if _open_acl_finish() did not return an
> > > > > error and state is DISCONNECTING, then 'err' is set, annd we will
> > > > > rerport an error". In particular, in the case when '_open_acl_finish() returned an
> > > > > error, and open_device() succeeded, no error will be returned'. In my
> > > > > opinion, it makes more sense as "if state is _DISCONNECTING regardless
> > > > > of acl_error, then return an error".
> > > > > 
> > > > > (let me know if I'm not making myself clear)
> > > > > 
> > > > > Christophe
> > > > 
> > > > And that's discussed in the latter part of the previous email you
> > > > trimmed. I missed this case when err is set by hand and removed the
> > > > if (err) part which skips the cancellation check.
> > > 
> > > No I think this is something different, think about acl_err being set,
> > > priv->state == STATE_DISCONNECTING, and _open_device() succeeding (so
> > > err is not set).
> > > 
> > Which is fixed by retaining the if (err) check around _open_device().
> 
> I don't think so. If _open_acl_finish() fails and priv->state is
> DISCONNECTING, and _open_device() succeeds:
> 
> Before:
>     spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &err);
>     if (!err && priv->state == STATE_DISCONNECTING) {
>         err = g_error_new_literal(G_IO_ERROR, G_IO_ERROR_CANCELLED,
>                                   "USB redirection channel connect cancelled");
>     }
>     if (!err) {
>         spice_usbredir_channel_open_device(channel, &err);
>     }
>     if (err) {
>         /* report error */
>     }
> 
> Here 'err' will be set after the _open_acl_finish() call, priv->state will not
> be checked (even though priv->state is DISCONNECTING), but _open_device() will
> not be attempted and an error will be reported.
> 
> After:
>     spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, &acl_err);
>     if (!acl_err && priv->state == STATE_DISCONNECTING) {
>         err = g_error_new_literal(G_IO_ERROR, G_IO_ERROR_CANCELLED,
>                                   "USB redirection channel connect cancelled");
>     }
> 
>     if (!err) {
>         spice_usbredir_channel_open_device(channel, &err);
>     }
>     if (err) {
>         /* report error */
>     }
> 
> acl_err will be non-NULL so priv->state will not be checked, open_device
> succeeds, no error reported.
> 
> 
> Maybe we can't get STATE_DISCONNECTING in the situation I describe, maybe we
> can get it, but this is not important, but I thought I'd ask :)

ok, so dropping the "!acl_err &&" part is needed in the case we get no
acl error and state is DISCONNECTING. It is also safe to do since
acl_err and err is separate.

Presumably this can happen if calling
spice_usb_device_manager_disconnect_device before
spice_usb_device_manager_connect_device_finish or the channel resets or
something like that. Probably not easily triggered with spicy.

Thanks

Michal