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

[Michal Suchanek]

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:
> > > 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.
> 
> 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.

Thanks

Michal