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

[Michal Suchanek]

Excerpts from Christophe Fergeau's message of Thu Jul 09 16:41:58 +0200 2015:
> On Thu, Jul 09, 2015 at 03:25:19PM +0100, Michal Suchanek wrote:
> > When calling ACL helper fails also try to open the device node directly.
> > 
> > Otherwise user-accessible device nodes are rejected when policykit
> > support is compiled in and policy is not set up when in fact the device
> > could be accessed.
> > 
> > Signed-off-by: Michal Suchanek <hramrach at gmail.com>
> > 
> > --
> > 
> > v2:
> > 
> >  - clear errors properly
> > ---
> >  src/channel-usbredir.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index 292b82f..7f4ddd7 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -276,27 +276,35 @@ static void spice_usbredir_channel_open_acl_cb(
> >      SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(user_data);
> >      SpiceUsbredirChannelPrivate *priv = channel->priv;
> >      GError *err = NULL;
> > +    GError *acl_err = NULL;
> >  
> >      g_return_if_fail(acl_helper == priv->acl_helper);
> >      g_return_if_fail(priv->state == STATE_WAITING_FOR_ACL_HELPER ||
> >                       priv->state == STATE_DISCONNECTING);
> >  
> > -    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.

> 
> >          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);
> > -    }
> > +
> > +    spice_usbredir_channel_open_device(channel, &err);
> 
> Ok, now that I reread the whole patch, err may be !NULL here if
> priv->state is STATE_DISCONNECTING so you need to keep the if (!err) {}
> around this call.

yes, err may be set as a result of cancelling the connection so the if
(err) should be kept.

Thanks

Michal

> 
> Christophe