>
> From: Victor Toso <me@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel