[Spice-devel] [spice-gtk 5/9] usb-redir: do not use spice_usb_acl_helper for emulated devices

[Yuri Benditovich]

On Fri, Jul 26, 2019 at 1:02 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> > On Fri, Jul 26, 2019 at 11:47 AM Frediano Ziglio <fziglio at redhat.com> wrote:
> > >
> > > > On Thu, Jul 25, 2019 at 12:17 PM Frediano Ziglio <fziglio at redhat.com>
> > > > wrote:
> > > > >
> > > > > >
> > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > > > > > ---
> > > > > >  src/channel-usbredir.c | 29 ++++++++++++++---------------
> > > > > >  1 file changed, 14 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > > > > > index 8d4cd66..961a464 100644
> > > > > > --- a/src/channel-usbredir.c
> > > > > > +++ b/src/channel-usbredir.c
> > > > > > @@ -301,7 +301,6 @@ static void spice_usbredir_channel_open_acl_cb(
> > > > > >  }
> > > > > >  #endif
> > > > > >
> > > > > > -#ifndef USE_POLKIT
> > > > > >  static void
> > > > > >  _open_device_async_cb(GTask *task,
> > > > > >                        gpointer object,
> > > > > > @@ -328,7 +327,6 @@ _open_device_async_cb(GTask *task,
> > > > > >          g_task_return_boolean(task, TRUE);
> > > > > >      }
> > > > > >  }
> > > > > > -#endif
> > > > > >
> > > > > >  G_GNUC_INTERNAL
> > > > > >  void spice_usbredir_channel_connect_device_async(
> > > > > > @@ -373,21 +371,22 @@ void
> > > > > > spice_usbredir_channel_connect_device_async(
> > > > > >      priv->spice_device = g_boxed_copy(spice_usb_device_get_type(),
> > > > > >                                        spice_device);
> > > > > >  #ifdef USE_POLKIT
> > > > > > -    priv->task = task;
> > > > > > -    priv->state  = STATE_WAITING_FOR_ACL_HELPER;
> > > > > > -    priv->acl_helper = spice_usb_acl_helper_new();
> > > > > > -    g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
> > > > > > -                 "inhibit-keyboard-grab", TRUE, NULL);
> > > > > > -    spice_usb_acl_helper_open_acl_async(priv->acl_helper,
> > > > > > -                                        info->bus,
> > > > > > -                                        info->address,
> > > > > > -                                        cancellable,
> > > > > > -
> > > > > > spice_usbredir_channel_open_acl_cb,
> > > > > > -                                        channel);
> > > > > > +    if (info->device_type == USB_DEV_TYPE_NONE) {
> > > > >
> > > > > Why not
> > > > >
> > > > >    if (info->device_type != USB_DEV_TYPE_NONE) {
> > > > >       return;
> > > > >    }
> > > > >
> > > >
> > > > Because this is not "return", this should proceed to g_task_run_in_thread
> > > > etc
> > > >
> > >
> > > In this case you have to remove the "return" just after the if.
> > >
> > > You either disabled POLKIT or you didn't test that code.
> >
> > I do not agree. POLKIT is enabled (on Linux).
> > With emulated device the code should work as without POLKIT.
> > For that the ifdef around _open_device_async_cb was removed and the
> > procedure is involved for emulated device.
> >
>
> The code with polkit enabled with your patches is:
>
>
>     priv->device = spice_usb_backend_device_ref(device);
>     priv->spice_device = g_boxed_copy(spice_usb_device_get_type(),
>                                       spice_device);
>     if (info->device_type == USB_DEV_TYPE_NONE) {
>         priv->task = task;
>         priv->state  = STATE_WAITING_FOR_ACL_HELPER;
>         priv->acl_helper = spice_usb_acl_helper_new();
>         g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
>                     "inhibit-keyboard-grab", TRUE, NULL);
>         spice_usb_acl_helper_open_acl_async(priv->acl_helper,
>                                             info->bus,
>                                             info->address,
>                                             cancellable,
>                                             spice_usbredir_channel_open_acl_cb,
>                                             channel);
>     }
>     return;

I've sent wrong version of the patch. My fault.
In my source tree the return is inside {}

>
>     g_task_run_in_thread(task, _open_device_async_cb);
>
> done:
>     g_object_unref(task);
> }
>
>
> Note that g_task_run_in_thread after the return is not executed.
> I suggested (omitting device_type changes)
>
>
>     priv->device = spice_usb_backend_device_ref(device);
>     priv->spice_device = g_boxed_copy(spice_usb_device_get_type(),
>                                       spice_device);
>     if (info->device_type != USB_DEV_TYPE_NONE) {
>         return;
>     }
>     priv->task = task;
>     priv->state  = STATE_WAITING_FOR_ACL_HELPER;
>     priv->acl_helper = spice_usb_acl_helper_new();
>     g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
>                  "inhibit-keyboard-grab", TRUE, NULL);
>     spice_usb_acl_helper_open_acl_async(priv->acl_helper,
>                                         info->bus,
>                                         info->address,
>                                         cancellable,
>                                         spice_usbredir_channel_open_acl_cb,
>                                         channel);
>     return;
>
> done:
>     g_object_unref(task);
> }
>
>
> Does not make much difference to me.
>
> > >
> > > > > would minimize changes.
> > > > > As stated in previous comment (other patch) the enumeration is
> > > > > misleading.
> > > > > As "info" hold information for any usb device people reading this would
> > > > > say
> > > > > that the device was not valid, something like
> > > > >
> > > > >   if (info->emulated_type != USB_DEV_EMU_TYPE_NOT_EMULATED)
> > > > >
> > > > > or
> > > > >
> > > > >   if (info->emulated_type != USB_DEV_EMU_TYPE_REAL)
> > > > >
> > > > > would be much more understandable.
> > > > >
> > > > > > +        priv->task = task;
> > > > > > +        priv->state  = STATE_WAITING_FOR_ACL_HELPER;
> > > > > > +        priv->acl_helper = spice_usb_acl_helper_new();
> > > > > > +
> > > > > > g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
> > > > > > +                    "inhibit-keyboard-grab", TRUE, NULL);
> > > > > > +        spice_usb_acl_helper_open_acl_async(priv->acl_helper,
> > > > > > +                                            info->bus,
> > > > > > +                                            info->address,
> > > > > > +                                            cancellable,
> > > > > > +
> > > > > > spice_usbredir_channel_open_acl_cb,
> > > > > > +                                            channel);
> > > > > > +    }
> > > > > >      return;
> > > > > > -#else
> > > > > > -    g_task_run_in_thread(task, _open_device_async_cb);
> > > > > >  #endif
> > > > > > +    g_task_run_in_thread(task, _open_device_async_cb);
> > > > > >
> > > > > >  done:
> > > > > >      g_object_unref(task);
> > > > >
> > > > > Frediano
> > > >
> >