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

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150709/454b5bd6/attachment.sig>