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

Submitted by Victor Toso on May 7, 2019, 8:56 a.m.

Details

Message ID 20190507085605.10054-2-victortoso@redhat.com
State New
Headers show
Series "polkit code to be on usb-acl-helper" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso May 7, 2019, 8:56 a.m.
From: Victor Toso <me@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.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/usb-acl-helper.c | 88 +++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 38 deletions(-)

Patch hide | download patch | download mbox

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";
+    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)) {
+        return FALSE;
+    }
+
+    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) {
+        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:

Comments

> 
> From: Victor Toso <me@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@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