[Spice-devel] [spice-gtk v1 1/4] usb-acl-helper: move exec of binary to its own function

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> Create helper exec_usb_acl_helper_bin() to handle the execution of
> spice-client-glib-usb-acl-helper binary. This bin is only compiled in
> spice-gtk if Polkit is enabled.
> 
> This is a preparation patch for enabling the build of usb-acl-helper.c
> when Polkit is disabled.

I don't know if this last sentence is useful, also I don't agree on
other patches but I consider this good.

> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/usb-acl-helper.c | 88 +++++++++++++++++++++++++-------------------
>  1 file changed, 50 insertions(+), 38 deletions(-)
> 
> diff --git a/src/usb-acl-helper.c b/src/usb-acl-helper.c
> index 186b86e..3145597 100644
> --- a/src/usb-acl-helper.c
> +++ b/src/usb-acl-helper.c
> @@ -154,6 +154,54 @@ static void helper_child_watch_cb(GPid pid, gint status,
> gpointer user_data)
>      /* Nothing to do, but we need the child watch to avoid zombies */
>  }
>  
> +static gboolean
> +exec_usb_acl_helper_bin(SpiceUsbAclHelper *self,
> +                        const gchar *buf,
> +                        GError **err)
> +{
> +    GIOStatus status;
> +    GPid helper_pid;
> +    gsize bytes_written;
> +    const gchar *acl_helper = g_getenv("SPICE_USB_ACL_BINARY");
> +    if (acl_helper == NULL)
> +        acl_helper = ACL_HELPER_PATH"/spice-client-glib-usb-acl-helper";

style here is weird. Looks like the statements between the declarations are
hidden to make the observer think are just declarations.

I would change to

   GIOStatus status;
   GPid helper_pid;
   gsize bytes_written;
   gint in, out;
   SpiceUsbAclHelperPrivate *priv = self->priv;
   const gchar *acl_helper = g_getenv("SPICE_USB_ACL_BINARY");

   if (acl_helper == NULL) {
       acl_helper = ACL_HELPER_PATH"/spice-client-glib-usb-acl-helper";
   }

   gchar *argv[] = { (char*)acl_helper, NULL };


> +    gchar *argv[] = { (char*)acl_helper, NULL };
> +    gint in, out;
> +    SpiceUsbAclHelperPrivate *priv = self->priv;
> +
> +    if (!g_spawn_async_with_pipes(NULL, argv, NULL,
> +                           G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_SEARCH_PATH,
> +                           NULL, NULL, &helper_pid, &in, &out, NULL, err)) {

I would fix the indentation here

> +        return FALSE;
> +    }
> +
> +    g_child_watch_add(helper_pid, helper_child_watch_cb, NULL);
> +
> +    priv->in_ch = g_io_channel_unix_new(in);

Not a regression, here you are assuming in_ch and out_ch are NULLs.
>From API prospective you cannot call spice_usb_acl_helper_open_acl_async
twice on the same object.

> +    g_io_channel_set_close_on_unref(priv->in_ch, TRUE);
> +
> +    priv->out_ch = g_io_channel_unix_new(out);
> +    g_io_channel_set_close_on_unref(priv->out_ch, TRUE);
> +    status = g_io_channel_set_flags(priv->out_ch, G_IO_FLAG_NONBLOCK, err);
> +    if (status != G_IO_STATUS_NORMAL) {
> +        return FALSE;
> +    }
> +
> +    status = g_io_channel_write_chars(priv->in_ch, buf, -1,
> +                                      &bytes_written, err);
> +    if (status != G_IO_STATUS_NORMAL) {
> +        return FALSE;
> +    }
> +    status = g_io_channel_flush(priv->in_ch, err);
> +    if (status != G_IO_STATUS_NORMAL) {
> +        return FALSE;
> +    }
> +
> +    g_io_add_watch(priv->out_ch, G_IO_IN|G_IO_HUP,
> +                   (GIOFunc)cb_out_watch, g_object_ref(self));
> +    return TRUE;
> +}
> +
>  /* ------------------------------------------------------------------ */
>  /* private api                                                        */
>  
> @@ -179,15 +227,6 @@ void
> spice_usb_acl_helper_open_acl_async(SpiceUsbAclHelper *self,
>      SpiceUsbAclHelperPrivate *priv = self->priv;
>      GTask *task;
>      GError *err = NULL;
> -    GIOStatus status;
> -    GPid helper_pid;
> -    gsize bytes_written;
> -    const gchar *acl_helper = g_getenv("SPICE_USB_ACL_BINARY");
> -    if (acl_helper == NULL)
> -        acl_helper = ACL_HELPER_PATH"/spice-client-glib-usb-acl-helper";
> -    gchar *argv[] = { (char*)acl_helper, NULL };
> -    gint in, out;
> -    gchar buf[128];
>  
>      task = g_task_new(self, cancellable, callback, user_data);
>  
> @@ -203,34 +242,9 @@ void
> spice_usb_acl_helper_open_acl_async(SpiceUsbAclHelper *self,
>          goto done;
>      }
>  
> -    if (!g_spawn_async_with_pipes(NULL, argv, NULL,
> -                           G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_SEARCH_PATH,
> -                           NULL, NULL, &helper_pid, &in, &out, NULL, &err))
> {
> -        g_task_return_error(task, err);
> -        goto done;
> -    }
> -    g_child_watch_add(helper_pid, helper_child_watch_cb, NULL);
> -
> -    priv->in_ch = g_io_channel_unix_new(in);
> -    g_io_channel_set_close_on_unref(priv->in_ch, TRUE);
> -
> -    priv->out_ch = g_io_channel_unix_new(out);
> -    g_io_channel_set_close_on_unref(priv->out_ch, TRUE);
> -    status = g_io_channel_set_flags(priv->out_ch, G_IO_FLAG_NONBLOCK, &err);
> -    if (status != G_IO_STATUS_NORMAL) {
> -        g_task_return_error(task, err);
> -        goto done;
> -    }
> -
> +    gchar buf[128];
>      snprintf(buf, sizeof(buf), "%d %d\n", busnum, devnum);
> -    status = g_io_channel_write_chars(priv->in_ch, buf, -1,
> -                                      &bytes_written, &err);
> -    if (status != G_IO_STATUS_NORMAL) {
> -        g_task_return_error(task, err);
> -        goto done;
> -    }
> -    status = g_io_channel_flush(priv->in_ch, &err);
> -    if (status != G_IO_STATUS_NORMAL) {
> +    if (!exec_usb_acl_helper_bin(self, buf, &err)) {
>          g_task_return_error(task, err);
>          goto done;
>      }
> @@ -242,8 +256,6 @@ void
> spice_usb_acl_helper_open_acl_async(SpiceUsbAclHelper *self,
>                                                       G_CALLBACK(cancelled_cb),
>                                                       self, NULL);
>      }
> -    g_io_add_watch(priv->out_ch, G_IO_IN|G_IO_HUP,
> -                   (GIOFunc)cb_out_watch, g_object_ref(self));
>      return;
>  
>  done:

Other part looks good

Frediano