[RFC,spice-vdagent,01/18] vdagentd: parse argv using GLib

Submitted by Jakub Janku on Aug. 14, 2018, 6:53 p.m.

Details

Message ID 20180814185352.6080-2-jjanku@redhat.com
State New
Headers show
Series "GLib integration" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jakub Janku Aug. 14, 2018, 6:53 p.m.
All command line options now have long names
as they are required by GLib.

Change types of some global variables that hold the options:
- const char * --> gchar *
- int          --> gboolean

Define DEFAULT_UINPUT_DEVICE as "/dev/uinput",
since there's already DEFAULT_VIRTIO_PORT_PATH, VDAGENTD_SOCKET.
---
 src/vdagentd/vdagentd.c | 149 ++++++++++++++++++++++------------------
 1 file changed, 82 insertions(+), 67 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 682761a..d88bbc7 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -48,6 +48,8 @@ 
 #include "virtio-port.h"
 #include "session-info.h"
 
+#define DEFAULT_UINPUT_DEVICE "/dev/uinput"
+
 struct agent_data {
     char *session;
     int width;
@@ -58,12 +60,16 @@  struct agent_data {
 
 /* variables */
 static const char *pidfilename = "/var/run/spice-vdagentd/spice-vdagentd.pid";
-static const char *portdev = DEFAULT_VIRTIO_PORT_PATH;
-static const char *vdagentd_socket = VDAGENTD_SOCKET;
-static const char *uinput_device = "/dev/uinput";
+
+static gchar *portdev = NULL;
+static gchar *vdagentd_socket = NULL;
+static gchar *uinput_device = NULL;
 static int debug = 0;
-static int uinput_fake = 0;
-static int only_once = 0;
+static gboolean uinput_fake = FALSE;
+static gboolean only_once = FALSE;
+static gboolean do_daemonize = TRUE;
+static gboolean want_session_info = TRUE;
+
 static struct udscs_server *server = NULL;
 static struct vdagent_virtio_port *virtio_port = NULL;
 static GHashTable *active_xfers = NULL;
@@ -960,29 +966,6 @@  static void agent_read_complete(struct udscs_connection **connp,
 
 /* main */
 
-static void usage(FILE *fp)
-{
-    fprintf(fp,
-            "Usage: spice-vdagentd [OPTIONS]\n\n"
-            "Spice guest agent daemon, version %s.\n\n"
-            "Options:\n"
-            "  -h             print this text\n"
-            "  -d             log debug messages (use twice for extra info)\n"
-            "  -s <port>      set virtio serial port  [%s]\n"
-            "  -S <filename>  set vdagent Unix domain socket [%s]\n"
-            "  -u <dev>       set uinput device       [%s]\n"
-            "  -f             treat uinput device as fake; no ioctls\n"
-            "  -x             don't daemonize\n"
-            "  -o             only handle one virtio serial session\n"
-#ifdef HAVE_CONSOLE_KIT
-            "  -X             disable console kit integration\n"
-#endif
-#ifdef HAVE_LIBSYSTEMD_LOGIN
-            "  -X             disable systemd-logind integration\n"
-#endif
-            ,VERSION, portdev, vdagentd_socket, uinput_device);
-}
-
 static void daemonize(void)
 {
     int x;
@@ -1081,52 +1064,80 @@  static void quit_handler(int sig)
     quit = 1;
 }
 
+static gboolean parse_debug_level_cb(const gchar *option_name,
+                                     const gchar *value,
+                                     gpointer     data,
+                                     GError     **error)
+{
+    debug++;
+    return TRUE;
+}
+
+static GOptionEntry cmd_entries[] = {
+    { "debug", 'd', G_OPTION_FLAG_NO_ARG,
+      G_OPTION_ARG_CALLBACK, parse_debug_level_cb,
+      "Log debug messages (use twice for extra info)", NULL },
+
+    { "virtio-serial-port-path", 's', 0,
+      G_OPTION_ARG_STRING, &portdev,
+      "Set virtio-serial path ("  DEFAULT_VIRTIO_PORT_PATH ")", NULL },
+
+    { "vdagentd-socket", 'S', 0,
+      G_OPTION_ARG_STRING, &vdagentd_socket,
+       "Set spice-vdagentd socket (" VDAGENTD_SOCKET ")", NULL },
+
+    { "uinput-device", 'u', 0,
+      G_OPTION_ARG_STRING, &uinput_device,
+      "Set uinput device (" DEFAULT_UINPUT_DEVICE ")", NULL },
+
+    { "fake-uinput", 'f', 0,
+      G_OPTION_ARG_NONE, &uinput_fake,
+      "Treat uinput device as fake; no ioctls", NULL },
+
+    { "foreground", 'x', G_OPTION_FLAG_REVERSE,
+      G_OPTION_ARG_NONE, &do_daemonize,
+      "Do not daemonize the agent", NULL},
+
+    { "one-session", 'o', 0,
+      G_OPTION_ARG_NONE, &only_once,
+      "Only handle one virtio serial session", NULL },
+
+#if defined(HAVE_CONSOLE_KIT) || defined (HAVE_LIBSYSTEMD_LOGIN)
+    { "disable-session-integration", 'X', G_OPTION_FLAG_REVERSE,
+      G_OPTION_ARG_NONE, &want_session_info,
+      "Disable console kit and systemd-logind integration", NULL },
+#endif
+
+    { NULL }
+};
+
 int main(int argc, char *argv[])
 {
-    int c;
-    int do_daemonize = 1;
-    int want_session_info = 1;
+    GOptionContext *context;
+    GError *err = NULL;
     struct sigaction act;
     gboolean own_socket = TRUE;
 
-    for (;;) {
-        if (-1 == (c = getopt(argc, argv, "-dhxXfos:u:S:")))
-            break;
-        switch (c) {
-        case 'd':
-            debug++;
-            break;
-        case 's':
-            portdev = optarg;
-            break;
-        case 'S':
-            vdagentd_socket = optarg;
-            break;
-        case 'u':
-            uinput_device = optarg;
-            break;
-        case 'f':
-            uinput_fake = 1;
-            break;
-        case 'o':
-            only_once = 1;
-            break;
-        case 'x':
-            do_daemonize = 0;
-            break;
-        case 'X':
-            want_session_info = 0;
-            break;
-        case 'h':
-            usage(stdout);
-            return 0;
-        default:
-            fputs("\n", stderr);
-            usage(stderr);
-            return 1;
-        }
+    context = g_option_context_new(NULL);
+    g_option_context_add_main_entries(context, cmd_entries, NULL);
+    g_option_context_set_summary(context,
+        "Spice guest agent daemon, version " VERSION);
+    g_option_context_parse(context, &argc, &argv, &err);
+    g_option_context_free(context);
+
+    if (err) {
+        g_printerr("Invalid arguments, %s\n", err->message);
+        g_error_free(err);
+        return 1;
     }
 
+    if (portdev == NULL)
+        portdev = g_strdup(DEFAULT_VIRTIO_PORT_PATH);
+    if (vdagentd_socket == NULL)
+        vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
+    if (uinput_device == NULL)
+        uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
+
     memset(&act, 0, sizeof(act));
     act.sa_flags = SA_RESTART;
     act.sa_handler = quit_handler;
@@ -1223,5 +1234,9 @@  int main(int argc, char *argv[])
     if (do_daemonize)
         unlink(pidfilename);
 
+    g_free(portdev);
+    g_free(vdagentd_socket);
+    g_free(uinput_device);
+
     return retval;
 }

Comments

Hi,

Just for reference, the output of spice-vdagentd -h before was:

 | $ spice-vdagentd -h
 | Usage: spice-vdagentd [OPTIONS]
 |
 | Spice guest agent daemon, version 0.17.0.
 |
 | Options:
 |   -h             print this text
 |   -d             log debug messages (use twice for extra info)
 |   -s <port>      set virtio serial port [/dev/virtio-ports/com.redhat.spice.0]
 |   -S <filename>  set udcs socket [/var/run/spice-vdagentd/spice-vdagent-sock]
 |   -u <dev>       set uinput device       [/dev/uinput]
 |   -f             treat uinput device as fake; no ioctls
 |   -x             don't daemonize
 |   -o             Only handle one virtio serial session.
 |   -X         Disable systemd-logind integration

and not it is:

 | $ /usr/sbin/spice-vdagentd -h
 | Usage:
 |   spice-vdagentd [OPTION?]
 | 
 |   Spice guest agent daemon, version 0.18.0
 | 
 |   Help Options:
 |     -h, --help                            Show help options
 | 
 | Application Options:
 |   -d, --debug                           Log debug messages (use twice for extra info)
 |   -s, --virtio-serial-port-path         Set virtio-serial path (/dev/virtio-ports/com.redhat.spice.0)
 |   -S, --vdagentd-socket                 Set spice-vdagentd socket (/var/run/spice-vdagentd/spice-vdagent-sock)
 |   -u, --uinput-device                   Set uinput device (/dev/uinput)
 |   -f, --fake-uinput                     Treat uinput device as fake; no ioctls
 |   -x, --foreground                      Do not daemonize the agent
 |   -o, --one-session                     Only handle one virtio serial session
 |   -X, --disable-session-integration     Disable console kit and systemd-logind integration

I think that after "vdagentd: use GMainLoop" patch, we should
also include glib's command line options.

On Tue, Aug 14, 2018 at 08:53:35PM +0200, Jakub Janků wrote:
> All command line options now have long names
> as they are required by GLib.

I think that adding the following to the commit log as a quick
overview of current options and its long names might be helpful.

    -d, --debug
    -s, --virtio-serial-port-path
    -S, --vdagentd-socket
    -u, --uinput-device
    -f, --fake-uinput
    -x, --foreground
    -o, --one-session
    -X, --disable-session-integration

> Change types of some global variables that hold the options:
> - const char * --> gchar *
> - int          --> gboolean
> 
> Define DEFAULT_UINPUT_DEVICE as "/dev/uinput",
> since there's already DEFAULT_VIRTIO_PORT_PATH, VDAGENTD_SOCKET.

Would you mind adding Signed-off-by: Name <email> ?

(more below)

> ---
>  src/vdagentd/vdagentd.c | 149 ++++++++++++++++++++++------------------
>  1 file changed, 82 insertions(+), 67 deletions(-)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 682761a..d88bbc7 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -48,6 +48,8 @@
>  #include "virtio-port.h"
>  #include "session-info.h"
>  
> +#define DEFAULT_UINPUT_DEVICE "/dev/uinput"
> +
>  struct agent_data {
>      char *session;
>      int width;
> @@ -58,12 +60,16 @@ struct agent_data {
>  
>  /* variables */
>  static const char *pidfilename = "/var/run/spice-vdagentd/spice-vdagentd.pid";
> -static const char *portdev = DEFAULT_VIRTIO_PORT_PATH;
> -static const char *vdagentd_socket = VDAGENTD_SOCKET;
> -static const char *uinput_device = "/dev/uinput";
> +
> +static gchar *portdev = NULL;
> +static gchar *vdagentd_socket = NULL;
> +static gchar *uinput_device = NULL;
>  static int debug = 0;
> -static int uinput_fake = 0;
> -static int only_once = 0;
> +static gboolean uinput_fake = FALSE;
> +static gboolean only_once = FALSE;
> +static gboolean do_daemonize = TRUE;
> +static gboolean want_session_info = TRUE;
> +
>  static struct udscs_server *server = NULL;
>  static struct vdagent_virtio_port *virtio_port = NULL;
>  static GHashTable *active_xfers = NULL;
> @@ -960,29 +966,6 @@ static void agent_read_complete(struct udscs_connection **connp,
>  
>  /* main */
>  
> -static void usage(FILE *fp)
> -{
> -    fprintf(fp,
> -            "Usage: spice-vdagentd [OPTIONS]\n\n"
> -            "Spice guest agent daemon, version %s.\n\n"
> -            "Options:\n"
> -            "  -h             print this text\n"
> -            "  -d             log debug messages (use twice for extra info)\n"
> -            "  -s <port>      set virtio serial port  [%s]\n"
> -            "  -S <filename>  set vdagent Unix domain socket [%s]\n"
> -            "  -u <dev>       set uinput device       [%s]\n"
> -            "  -f             treat uinput device as fake; no ioctls\n"
> -            "  -x             don't daemonize\n"
> -            "  -o             only handle one virtio serial session\n"
> -#ifdef HAVE_CONSOLE_KIT
> -            "  -X             disable console kit integration\n"
> -#endif
> -#ifdef HAVE_LIBSYSTEMD_LOGIN
> -            "  -X             disable systemd-logind integration\n"
> -#endif
> -            ,VERSION, portdev, vdagentd_socket, uinput_device);
> -}
> -
>  static void daemonize(void)
>  {
>      int x;
> @@ -1081,52 +1064,80 @@ static void quit_handler(int sig)
>      quit = 1;
>  }
>  
> +static gboolean parse_debug_level_cb(const gchar *option_name,
> +                                     const gchar *value,
> +                                     gpointer     data,
> +                                     GError     **error)
> +{
> +    debug++;
> +    return TRUE;
> +}

It is okay to leave this for now as this patches are touching the
command line parsing part but within the context of glib
integration, we might consider removing this in the future and
follow what might be dictated by Glib's logging system.

> +static GOptionEntry cmd_entries[] = {
> +    { "debug", 'd', G_OPTION_FLAG_NO_ARG,
> +      G_OPTION_ARG_CALLBACK, parse_debug_level_cb,
> +      "Log debug messages (use twice for extra info)", NULL },
> +
> +    { "virtio-serial-port-path", 's', 0,
> +      G_OPTION_ARG_STRING, &portdev,
> +      "Set virtio-serial path ("  DEFAULT_VIRTIO_PORT_PATH ")", NULL },
> +
> +    { "vdagentd-socket", 'S', 0,
> +      G_OPTION_ARG_STRING, &vdagentd_socket,
> +       "Set spice-vdagentd socket (" VDAGENTD_SOCKET ")", NULL },
> +
> +    { "uinput-device", 'u', 0,
> +      G_OPTION_ARG_STRING, &uinput_device,
> +      "Set uinput device (" DEFAULT_UINPUT_DEVICE ")", NULL },
> +
> +    { "fake-uinput", 'f', 0,
> +      G_OPTION_ARG_NONE, &uinput_fake,
> +      "Treat uinput device as fake; no ioctls", NULL },
> +
> +    { "foreground", 'x', G_OPTION_FLAG_REVERSE,
> +      G_OPTION_ARG_NONE, &do_daemonize,
> +      "Do not daemonize the agent", NULL},
> +
> +    { "one-session", 'o', 0,
> +      G_OPTION_ARG_NONE, &only_once,
> +      "Only handle one virtio serial session", NULL },
> +
> +#if defined(HAVE_CONSOLE_KIT) || defined (HAVE_LIBSYSTEMD_LOGIN)
> +    { "disable-session-integration", 'X', G_OPTION_FLAG_REVERSE,
> +      G_OPTION_ARG_NONE, &want_session_info,
> +      "Disable console kit and systemd-logind integration", NULL },
> +#endif
> +
> +    { NULL }
> +};
> +
>  int main(int argc, char *argv[])
>  {
> -    int c;
> -    int do_daemonize = 1;
> -    int want_session_info = 1;
> +    GOptionContext *context;
> +    GError *err = NULL;
>      struct sigaction act;
>      gboolean own_socket = TRUE;
>  
> -    for (;;) {
> -        if (-1 == (c = getopt(argc, argv, "-dhxXfos:u:S:")))
> -            break;
> -        switch (c) {
> -        case 'd':
> -            debug++;
> -            break;
> -        case 's':
> -            portdev = optarg;
> -            break;
> -        case 'S':
> -            vdagentd_socket = optarg;
> -            break;
> -        case 'u':
> -            uinput_device = optarg;
> -            break;
> -        case 'f':
> -            uinput_fake = 1;
> -            break;
> -        case 'o':
> -            only_once = 1;
> -            break;
> -        case 'x':
> -            do_daemonize = 0;
> -            break;
> -        case 'X':
> -            want_session_info = 0;
> -            break;
> -        case 'h':
> -            usage(stdout);
> -            return 0;
> -        default:
> -            fputs("\n", stderr);
> -            usage(stderr);
> -            return 1;
> -        }
> +    context = g_option_context_new(NULL);
> +    g_option_context_add_main_entries(context, cmd_entries, NULL);
> +    g_option_context_set_summary(context,
> +        "Spice guest agent daemon, version " VERSION);
> +    g_option_context_parse(context, &argc, &argv, &err);
> +    g_option_context_free(context);
> +
> +    if (err) {
> +        g_printerr("Invalid arguments, %s\n", err->message);

We don't use g_printerr() or any g_log() in this code yet.

I hope we can move to that whenever we bump glib to 2.50 or above
as it writes to systemd's journal by default for daemons (AFAIK).

Reviewed-by: Victor Toso <victortoso@redhat.com>

Feel free to submit a new version with fixes as standalone patch
with me in cc, to reduce the backlog of this series ;)

Cheers,
Victor

> +        g_error_free(err);
> +        return 1;
>      }
>  
> +    if (portdev == NULL)
> +        portdev = g_strdup(DEFAULT_VIRTIO_PORT_PATH);
> +    if (vdagentd_socket == NULL)
> +        vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
> +    if (uinput_device == NULL)
> +        uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
> +
>      memset(&act, 0, sizeof(act));
>      act.sa_flags = SA_RESTART;
>      act.sa_handler = quit_handler;
> @@ -1223,5 +1234,9 @@ int main(int argc, char *argv[])
>      if (do_daemonize)
>          unlink(pidfilename);
>  
> +    g_free(portdev);
> +    g_free(vdagentd_socket);
> +    g_free(uinput_device);
> +
>      return retval;
>  }
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Tue, Aug 28, 2018 at 7:16 AM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> Just for reference, the output of spice-vdagentd -h before was:
>
>  | $ spice-vdagentd -h
>  | Usage: spice-vdagentd [OPTIONS]
>  |
>  | Spice guest agent daemon, version 0.17.0.
>  |
>  | Options:
>  |   -h             print this text
>  |   -d             log debug messages (use twice for extra info)
>  |   -s <port>      set virtio serial port [/dev/virtio-ports/com.redhat.spice.0]
>  |   -S <filename>  set udcs socket [/var/run/spice-vdagentd/spice-vdagent-sock]
>  |   -u <dev>       set uinput device       [/dev/uinput]
>  |   -f             treat uinput device as fake; no ioctls
>  |   -x             don't daemonize
>  |   -o             Only handle one virtio serial session.
>  |   -X         Disable systemd-logind integration
>
> and not it is:
>
>  | $ /usr/sbin/spice-vdagentd -h
>  | Usage:
>  |   spice-vdagentd [OPTION?]
>  |
>  |   Spice guest agent daemon, version 0.18.0
>  |
>  |   Help Options:
>  |     -h, --help                            Show help options
>  |
>  | Application Options:
>  |   -d, --debug                           Log debug messages (use twice for extra info)
>  |   -s, --virtio-serial-port-path         Set virtio-serial path (/dev/virtio-ports/com.redhat.spice.0)
>  |   -S, --vdagentd-socket                 Set spice-vdagentd socket (/var/run/spice-vdagentd/spice-vdagent-sock)
>  |   -u, --uinput-device                   Set uinput device (/dev/uinput)
>  |   -f, --fake-uinput                     Treat uinput device as fake; no ioctls
>  |   -x, --foreground                      Do not daemonize the agent
>  |   -o, --one-session                     Only handle one virtio serial session
>  |   -X, --disable-session-integration     Disable console kit and systemd-logind integration
>
> I think that after "vdagentd: use GMainLoop" patch, we should
> also include glib's command line options.

Not sure what you are referring to here.
GLib uses environment variables instead of command line options,
AFAIK, doesn't it?
There's the gtk_get_option_group() that we use in vdagent.c, but I
don't know of anything similar for GLib.
>
> On Tue, Aug 14, 2018 at 08:53:35PM +0200, Jakub Janků wrote:
> > All command line options now have long names
> > as they are required by GLib.
>
> I think that adding the following to the commit log as a quick
> overview of current options and its long names might be helpful.

Sure.
>
>     -d, --debug
>     -s, --virtio-serial-port-path
>     -S, --vdagentd-socket
>     -u, --uinput-device
>     -f, --fake-uinput
>     -x, --foreground
>     -o, --one-session
>     -X, --disable-session-integration
>
> > Change types of some global variables that hold the options:
> > - const char * --> gchar *
> > - int          --> gboolean
> >
> > Define DEFAULT_UINPUT_DEVICE as "/dev/uinput",
> > since there's already DEFAULT_VIRTIO_PORT_PATH, VDAGENTD_SOCKET.
>
> Would you mind adding Signed-off-by: Name <email> ?
>
> (more below)
>
> > ---
> >  src/vdagentd/vdagentd.c | 149 ++++++++++++++++++++++------------------
> >  1 file changed, 82 insertions(+), 67 deletions(-)
> >
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index 682761a..d88bbc7 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -48,6 +48,8 @@
> >  #include "virtio-port.h"
> >  #include "session-info.h"
> >
> > +#define DEFAULT_UINPUT_DEVICE "/dev/uinput"
> > +
> >  struct agent_data {
> >      char *session;
> >      int width;
> > @@ -58,12 +60,16 @@ struct agent_data {
> >
> >  /* variables */
> >  static const char *pidfilename = "/var/run/spice-vdagentd/spice-vdagentd.pid";
> > -static const char *portdev = DEFAULT_VIRTIO_PORT_PATH;
> > -static const char *vdagentd_socket = VDAGENTD_SOCKET;
> > -static const char *uinput_device = "/dev/uinput";
> > +
> > +static gchar *portdev = NULL;
> > +static gchar *vdagentd_socket = NULL;
> > +static gchar *uinput_device = NULL;
> >  static int debug = 0;
> > -static int uinput_fake = 0;
> > -static int only_once = 0;
> > +static gboolean uinput_fake = FALSE;
> > +static gboolean only_once = FALSE;
> > +static gboolean do_daemonize = TRUE;
> > +static gboolean want_session_info = TRUE;
> > +
> >  static struct udscs_server *server = NULL;
> >  static struct vdagent_virtio_port *virtio_port = NULL;
> >  static GHashTable *active_xfers = NULL;
> > @@ -960,29 +966,6 @@ static void agent_read_complete(struct udscs_connection **connp,
> >
> >  /* main */
> >
> > -static void usage(FILE *fp)
> > -{
> > -    fprintf(fp,
> > -            "Usage: spice-vdagentd [OPTIONS]\n\n"
> > -            "Spice guest agent daemon, version %s.\n\n"
> > -            "Options:\n"
> > -            "  -h             print this text\n"
> > -            "  -d             log debug messages (use twice for extra info)\n"
> > -            "  -s <port>      set virtio serial port  [%s]\n"
> > -            "  -S <filename>  set vdagent Unix domain socket [%s]\n"
> > -            "  -u <dev>       set uinput device       [%s]\n"
> > -            "  -f             treat uinput device as fake; no ioctls\n"
> > -            "  -x             don't daemonize\n"
> > -            "  -o             only handle one virtio serial session\n"
> > -#ifdef HAVE_CONSOLE_KIT
> > -            "  -X             disable console kit integration\n"
> > -#endif
> > -#ifdef HAVE_LIBSYSTEMD_LOGIN
> > -            "  -X             disable systemd-logind integration\n"
> > -#endif
> > -            ,VERSION, portdev, vdagentd_socket, uinput_device);
> > -}
> > -
> >  static void daemonize(void)
> >  {
> >      int x;
> > @@ -1081,52 +1064,80 @@ static void quit_handler(int sig)
> >      quit = 1;
> >  }
> >
> > +static gboolean parse_debug_level_cb(const gchar *option_name,
> > +                                     const gchar *value,
> > +                                     gpointer     data,
> > +                                     GError     **error)
> > +{
> > +    debug++;
> > +    return TRUE;
> > +}
>
> It is okay to leave this for now as this patches are touching the
> command line parsing part but within the context of glib
> integration, we might consider removing this in the future and
> follow what might be dictated by Glib's logging system.

This parse function is necessary for the command line options to stay the same.
The higher debug level is used in vdagentd_uinput_create(..., debug > 1, ...).
So removing this function would require adding a new option.
I would do this in a separate patch.
>
> > +static GOptionEntry cmd_entries[] = {
> > +    { "debug", 'd', G_OPTION_FLAG_NO_ARG,
> > +      G_OPTION_ARG_CALLBACK, parse_debug_level_cb,
> > +      "Log debug messages (use twice for extra info)", NULL },
> > +
> > +    { "virtio-serial-port-path", 's', 0,
> > +      G_OPTION_ARG_STRING, &portdev,
> > +      "Set virtio-serial path ("  DEFAULT_VIRTIO_PORT_PATH ")", NULL },
> > +
> > +    { "vdagentd-socket", 'S', 0,
> > +      G_OPTION_ARG_STRING, &vdagentd_socket,
> > +       "Set spice-vdagentd socket (" VDAGENTD_SOCKET ")", NULL },
> > +
> > +    { "uinput-device", 'u', 0,
> > +      G_OPTION_ARG_STRING, &uinput_device,
> > +      "Set uinput device (" DEFAULT_UINPUT_DEVICE ")", NULL },
> > +
> > +    { "fake-uinput", 'f', 0,
> > +      G_OPTION_ARG_NONE, &uinput_fake,
> > +      "Treat uinput device as fake; no ioctls", NULL },
> > +
> > +    { "foreground", 'x', G_OPTION_FLAG_REVERSE,
> > +      G_OPTION_ARG_NONE, &do_daemonize,
> > +      "Do not daemonize the agent", NULL},
> > +
> > +    { "one-session", 'o', 0,
> > +      G_OPTION_ARG_NONE, &only_once,
> > +      "Only handle one virtio serial session", NULL },
> > +
> > +#if defined(HAVE_CONSOLE_KIT) || defined (HAVE_LIBSYSTEMD_LOGIN)
> > +    { "disable-session-integration", 'X', G_OPTION_FLAG_REVERSE,
> > +      G_OPTION_ARG_NONE, &want_session_info,
> > +      "Disable console kit and systemd-logind integration", NULL },
> > +#endif
> > +
> > +    { NULL }
> > +};
> > +
> >  int main(int argc, char *argv[])
> >  {
> > -    int c;
> > -    int do_daemonize = 1;
> > -    int want_session_info = 1;
> > +    GOptionContext *context;
> > +    GError *err = NULL;
> >      struct sigaction act;
> >      gboolean own_socket = TRUE;
> >
> > -    for (;;) {
> > -        if (-1 == (c = getopt(argc, argv, "-dhxXfos:u:S:")))
> > -            break;
> > -        switch (c) {
> > -        case 'd':
> > -            debug++;
> > -            break;
> > -        case 's':
> > -            portdev = optarg;
> > -            break;
> > -        case 'S':
> > -            vdagentd_socket = optarg;
> > -            break;
> > -        case 'u':
> > -            uinput_device = optarg;
> > -            break;
> > -        case 'f':
> > -            uinput_fake = 1;
> > -            break;
> > -        case 'o':
> > -            only_once = 1;
> > -            break;
> > -        case 'x':
> > -            do_daemonize = 0;
> > -            break;
> > -        case 'X':
> > -            want_session_info = 0;
> > -            break;
> > -        case 'h':
> > -            usage(stdout);
> > -            return 0;
> > -        default:
> > -            fputs("\n", stderr);
> > -            usage(stderr);
> > -            return 1;
> > -        }
> > +    context = g_option_context_new(NULL);
> > +    g_option_context_add_main_entries(context, cmd_entries, NULL);
> > +    g_option_context_set_summary(context,
> > +        "Spice guest agent daemon, version " VERSION);
> > +    g_option_context_parse(context, &argc, &argv, &err);
> > +    g_option_context_free(context);
> > +
> > +    if (err) {
> > +        g_printerr("Invalid arguments, %s\n", err->message);
>
> We don't use g_printerr() or any g_log() in this code yet.

I think I copied it from the vdagent.c where it was added in 0ffca2b
("vdagent: Use glib's commandline parser").
I will change it to syslog().
>
> I hope we can move to that whenever we bump glib to 2.50 or above
> as it writes to systemd's journal by default for daemons (AFAIK).
>
> Reviewed-by: Victor Toso <victortoso@redhat.com>
>
> Feel free to submit a new version with fixes as standalone patch
> with me in cc, to reduce the backlog of this series ;)
>
> Cheers,
> Victor
>
> > +        g_error_free(err);
> > +        return 1;
> >      }
> >
> > +    if (portdev == NULL)
> > +        portdev = g_strdup(DEFAULT_VIRTIO_PORT_PATH);
> > +    if (vdagentd_socket == NULL)
> > +        vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
> > +    if (uinput_device == NULL)
> > +        uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
> > +
> >      memset(&act, 0, sizeof(act));
> >      act.sa_flags = SA_RESTART;
> >      act.sa_handler = quit_handler;
> > @@ -1223,5 +1234,9 @@ int main(int argc, char *argv[])
> >      if (do_daemonize)
> >          unlink(pidfilename);
> >
> > +    g_free(portdev);
> > +    g_free(vdagentd_socket);
> > +    g_free(uinput_device);
> > +
> >      return retval;
> >  }
> > --
> > 2.17.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel

Cheers,
Jakub
Hi,

On Mon, Sep 03, 2018 at 06:06:01PM +0200, Jakub Janku wrote:
> Hi,
> 
> > I think that after "vdagentd: use GMainLoop" patch, we should
> > also include glib's command line options.
> 
> Not sure what you are referring to here.
> GLib uses environment variables instead of command line
> options, AFAIK, doesn't it?
> There's the gtk_get_option_group() that we use in vdagent.c,
> but I don't know of anything similar for GLib.

Yep, there isn't :)
I probably got confused with gtk/glib at the time.

> > On Tue, Aug 14, 2018 at 08:53:35PM +0200, Jakub Janků wrote:
> > > All command line options now have long names
> > > as they are required by GLib.
> >
> > I think that adding the following to the commit log as a quick
> > overview of current options and its long names might be helpful.
> 
> Sure.
> >
> >     -d, --debug
> >     -s, --virtio-serial-port-path
> >     -S, --vdagentd-socket
> >     -u, --uinput-device
> >     -f, --fake-uinput
> >     -x, --foreground
> >     -o, --one-session
> >     -X, --disable-session-integration
> >
> > > Change types of some global variables that hold the options:
> > > - const char * --> gchar *
> > > - int          --> gboolean
> > >
> > > Define DEFAULT_UINPUT_DEVICE as "/dev/uinput",
> > > since there's already DEFAULT_VIRTIO_PORT_PATH, VDAGENTD_SOCKET.
> >
> > Would you mind adding Signed-off-by: Name <email> ?
> >
> > (more below)
> >
> > > ---
> > >  src/vdagentd/vdagentd.c | 149 ++++++++++++++++++++++------------------
> > >  1 file changed, 82 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > > index 682761a..d88bbc7 100644
> > > --- a/src/vdagentd/vdagentd.c
> > > +++ b/src/vdagentd/vdagentd.c
> > > @@ -48,6 +48,8 @@
> > >  #include "virtio-port.h"
> > >  #include "session-info.h"
> > >
> > > +#define DEFAULT_UINPUT_DEVICE "/dev/uinput"
> > > +
> > >  struct agent_data {
> > >      char *session;
> > >      int width;
> > > @@ -58,12 +60,16 @@ struct agent_data {
> > >
> > >  /* variables */
> > >  static const char *pidfilename = "/var/run/spice-vdagentd/spice-vdagentd.pid";
> > > -static const char *portdev = DEFAULT_VIRTIO_PORT_PATH;
> > > -static const char *vdagentd_socket = VDAGENTD_SOCKET;
> > > -static const char *uinput_device = "/dev/uinput";
> > > +
> > > +static gchar *portdev = NULL;
> > > +static gchar *vdagentd_socket = NULL;
> > > +static gchar *uinput_device = NULL;
> > >  static int debug = 0;
> > > -static int uinput_fake = 0;
> > > -static int only_once = 0;
> > > +static gboolean uinput_fake = FALSE;
> > > +static gboolean only_once = FALSE;
> > > +static gboolean do_daemonize = TRUE;
> > > +static gboolean want_session_info = TRUE;
> > > +
> > >  static struct udscs_server *server = NULL;
> > >  static struct vdagent_virtio_port *virtio_port = NULL;
> > >  static GHashTable *active_xfers = NULL;
> > > @@ -960,29 +966,6 @@ static void agent_read_complete(struct udscs_connection **connp,
> > >
> > >  /* main */
> > >
> > > -static void usage(FILE *fp)
> > > -{
> > > -    fprintf(fp,
> > > -            "Usage: spice-vdagentd [OPTIONS]\n\n"
> > > -            "Spice guest agent daemon, version %s.\n\n"
> > > -            "Options:\n"
> > > -            "  -h             print this text\n"
> > > -            "  -d             log debug messages (use twice for extra info)\n"
> > > -            "  -s <port>      set virtio serial port  [%s]\n"
> > > -            "  -S <filename>  set vdagent Unix domain socket [%s]\n"
> > > -            "  -u <dev>       set uinput device       [%s]\n"
> > > -            "  -f             treat uinput device as fake; no ioctls\n"
> > > -            "  -x             don't daemonize\n"
> > > -            "  -o             only handle one virtio serial session\n"
> > > -#ifdef HAVE_CONSOLE_KIT
> > > -            "  -X             disable console kit integration\n"
> > > -#endif
> > > -#ifdef HAVE_LIBSYSTEMD_LOGIN
> > > -            "  -X             disable systemd-logind integration\n"
> > > -#endif
> > > -            ,VERSION, portdev, vdagentd_socket, uinput_device);
> > > -}
> > > -
> > >  static void daemonize(void)
> > >  {
> > >      int x;
> > > @@ -1081,52 +1064,80 @@ static void quit_handler(int sig)
> > >      quit = 1;
> > >  }
> > >
> > > +static gboolean parse_debug_level_cb(const gchar *option_name,
> > > +                                     const gchar *value,
> > > +                                     gpointer     data,
> > > +                                     GError     **error)
> > > +{
> > > +    debug++;
> > > +    return TRUE;
> > > +}
> >
> > It is okay to leave this for now as this patches are touching the
> > command line parsing part but within the context of glib
> > integration, we might consider removing this in the future and
> > follow what might be dictated by Glib's logging system.
> 
> This parse function is necessary for the command line options
> to stay the same.  The higher debug level is used in
> vdagentd_uinput_create(..., debug > 1, ...).  So removing this
> function would require adding a new option.  I would do this in
> a separate patch.

Yes, no need to do it now. It was more like a comment for future
enhancements. This change would need glib's output to go to
systemd journal fields by default and I think that's the case
only with glib 2.50, but I could be wrong.

> > > +static GOptionEntry cmd_entries[] = {
> > > +    { "debug", 'd', G_OPTION_FLAG_NO_ARG,
> > > +      G_OPTION_ARG_CALLBACK, parse_debug_level_cb,
> > > +      "Log debug messages (use twice for extra info)", NULL },
> > > +
> > > +    { "virtio-serial-port-path", 's', 0,
> > > +      G_OPTION_ARG_STRING, &portdev,
> > > +      "Set virtio-serial path ("  DEFAULT_VIRTIO_PORT_PATH ")", NULL },
> > > +
> > > +    { "vdagentd-socket", 'S', 0,
> > > +      G_OPTION_ARG_STRING, &vdagentd_socket,
> > > +       "Set spice-vdagentd socket (" VDAGENTD_SOCKET ")", NULL },
> > > +
> > > +    { "uinput-device", 'u', 0,
> > > +      G_OPTION_ARG_STRING, &uinput_device,
> > > +      "Set uinput device (" DEFAULT_UINPUT_DEVICE ")", NULL },
> > > +
> > > +    { "fake-uinput", 'f', 0,
> > > +      G_OPTION_ARG_NONE, &uinput_fake,
> > > +      "Treat uinput device as fake; no ioctls", NULL },
> > > +
> > > +    { "foreground", 'x', G_OPTION_FLAG_REVERSE,
> > > +      G_OPTION_ARG_NONE, &do_daemonize,
> > > +      "Do not daemonize the agent", NULL},
> > > +
> > > +    { "one-session", 'o', 0,
> > > +      G_OPTION_ARG_NONE, &only_once,
> > > +      "Only handle one virtio serial session", NULL },
> > > +
> > > +#if defined(HAVE_CONSOLE_KIT) || defined (HAVE_LIBSYSTEMD_LOGIN)
> > > +    { "disable-session-integration", 'X', G_OPTION_FLAG_REVERSE,
> > > +      G_OPTION_ARG_NONE, &want_session_info,
> > > +      "Disable console kit and systemd-logind integration", NULL },
> > > +#endif
> > > +
> > > +    { NULL }
> > > +};
> > > +
> > >  int main(int argc, char *argv[])
> > >  {
> > > -    int c;
> > > -    int do_daemonize = 1;
> > > -    int want_session_info = 1;
> > > +    GOptionContext *context;
> > > +    GError *err = NULL;
> > >      struct sigaction act;
> > >      gboolean own_socket = TRUE;
> > >
> > > -    for (;;) {
> > > -        if (-1 == (c = getopt(argc, argv, "-dhxXfos:u:S:")))
> > > -            break;
> > > -        switch (c) {
> > > -        case 'd':
> > > -            debug++;
> > > -            break;
> > > -        case 's':
> > > -            portdev = optarg;
> > > -            break;
> > > -        case 'S':
> > > -            vdagentd_socket = optarg;
> > > -            break;
> > > -        case 'u':
> > > -            uinput_device = optarg;
> > > -            break;
> > > -        case 'f':
> > > -            uinput_fake = 1;
> > > -            break;
> > > -        case 'o':
> > > -            only_once = 1;
> > > -            break;
> > > -        case 'x':
> > > -            do_daemonize = 0;
> > > -            break;
> > > -        case 'X':
> > > -            want_session_info = 0;
> > > -            break;
> > > -        case 'h':
> > > -            usage(stdout);
> > > -            return 0;
> > > -        default:
> > > -            fputs("\n", stderr);
> > > -            usage(stderr);
> > > -            return 1;
> > > -        }
> > > +    context = g_option_context_new(NULL);
> > > +    g_option_context_add_main_entries(context, cmd_entries, NULL);
> > > +    g_option_context_set_summary(context,
> > > +        "Spice guest agent daemon, version " VERSION);
> > > +    g_option_context_parse(context, &argc, &argv, &err);
> > > +    g_option_context_free(context);
> > > +
> > > +    if (err) {
> > > +        g_printerr("Invalid arguments, %s\n", err->message);
> >
> > We don't use g_printerr() or any g_log() in this code yet.
> 
> I think I copied it from the vdagent.c where it was added in 0ffca2b
> ("vdagent: Use glib's commandline parser").
> I will change it to syslog().

Thanks,
Victor
> >
> > I hope we can move to that whenever we bump glib to 2.50 or above
> > as it writes to systemd's journal by default for daemons (AFAIK).
> >
> > Reviewed-by: Victor Toso <victortoso@redhat.com>
> >
> > Feel free to submit a new version with fixes as standalone patch
> > with me in cc, to reduce the backlog of this series ;)
> >
> > Cheers,
> > Victor
> >
> > > +        g_error_free(err);
> > > +        return 1;
> > >      }
> > >
> > > +    if (portdev == NULL)
> > > +        portdev = g_strdup(DEFAULT_VIRTIO_PORT_PATH);
> > > +    if (vdagentd_socket == NULL)
> > > +        vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
> > > +    if (uinput_device == NULL)
> > > +        uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
> > > +
> > >      memset(&act, 0, sizeof(act));
> > >      act.sa_flags = SA_RESTART;
> > >      act.sa_handler = quit_handler;
> > > @@ -1223,5 +1234,9 @@ int main(int argc, char *argv[])
> > >      if (do_daemonize)
> > >          unlink(pidfilename);
> > >
> > > +    g_free(portdev);
> > > +    g_free(vdagentd_socket);
> > > +    g_free(uinput_device);
> > > +
> > >      return retval;
> > >  }
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> Cheers,
> Jakub
On Tue, Sep 4, 2018 at 7:02 AM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> On Mon, Sep 03, 2018 at 06:06:01PM +0200, Jakub Janku wrote:
> > Hi,
> >
> > > I think that after "vdagentd: use GMainLoop" patch, we should
> > > also include glib's command line options.
> >
> > Not sure what you are referring to here.
> > GLib uses environment variables instead of command line
> > options, AFAIK, doesn't it?
> > There's the gtk_get_option_group() that we use in vdagent.c,
> > but I don't know of anything similar for GLib.
>
> Yep, there isn't :)
> I probably got confused with gtk/glib at the time.
>
> > > On Tue, Aug 14, 2018 at 08:53:35PM +0200, Jakub Janků wrote:
> > > > All command line options now have long names
> > > > as they are required by GLib.
> > >
> > > I think that adding the following to the commit log as a quick
> > > overview of current options and its long names might be helpful.
> >
> > Sure.
> > >
> > >     -d, --debug
> > >     -s, --virtio-serial-port-path
> > >     -S, --vdagentd-socket
> > >     -u, --uinput-device
> > >     -f, --fake-uinput
> > >     -x, --foreground
> > >     -o, --one-session
> > >     -X, --disable-session-integration
> > >
> > > > Change types of some global variables that hold the options:
> > > > - const char * --> gchar *
> > > > - int          --> gboolean
> > > >
> > > > Define DEFAULT_UINPUT_DEVICE as "/dev/uinput",
> > > > since there's already DEFAULT_VIRTIO_PORT_PATH, VDAGENTD_SOCKET.
> > >
> > > Would you mind adding Signed-off-by: Name <email> ?
> > >
> > > (more below)
> > >
> > > > ---
> > > >  src/vdagentd/vdagentd.c | 149 ++++++++++++++++++++++------------------
> > > >  1 file changed, 82 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > > > index 682761a..d88bbc7 100644
> > > > --- a/src/vdagentd/vdagentd.c
> > > > +++ b/src/vdagentd/vdagentd.c
> > > > @@ -48,6 +48,8 @@
> > > >  #include "virtio-port.h"
> > > >  #include "session-info.h"
> > > >
> > > > +#define DEFAULT_UINPUT_DEVICE "/dev/uinput"
> > > > +
> > > >  struct agent_data {
> > > >      char *session;
> > > >      int width;
> > > > @@ -58,12 +60,16 @@ struct agent_data {
> > > >
> > > >  /* variables */
> > > >  static const char *pidfilename = "/var/run/spice-vdagentd/spice-vdagentd.pid";
> > > > -static const char *portdev = DEFAULT_VIRTIO_PORT_PATH;
> > > > -static const char *vdagentd_socket = VDAGENTD_SOCKET;
> > > > -static const char *uinput_device = "/dev/uinput";
> > > > +
> > > > +static gchar *portdev = NULL;
> > > > +static gchar *vdagentd_socket = NULL;
> > > > +static gchar *uinput_device = NULL;
> > > >  static int debug = 0;
> > > > -static int uinput_fake = 0;
> > > > -static int only_once = 0;
> > > > +static gboolean uinput_fake = FALSE;
> > > > +static gboolean only_once = FALSE;
> > > > +static gboolean do_daemonize = TRUE;
> > > > +static gboolean want_session_info = TRUE;
> > > > +
> > > >  static struct udscs_server *server = NULL;
> > > >  static struct vdagent_virtio_port *virtio_port = NULL;
> > > >  static GHashTable *active_xfers = NULL;
> > > > @@ -960,29 +966,6 @@ static void agent_read_complete(struct udscs_connection **connp,
> > > >
> > > >  /* main */
> > > >
> > > > -static void usage(FILE *fp)
> > > > -{
> > > > -    fprintf(fp,
> > > > -            "Usage: spice-vdagentd [OPTIONS]\n\n"
> > > > -            "Spice guest agent daemon, version %s.\n\n"
> > > > -            "Options:\n"
> > > > -            "  -h             print this text\n"
> > > > -            "  -d             log debug messages (use twice for extra info)\n"
> > > > -            "  -s <port>      set virtio serial port  [%s]\n"
> > > > -            "  -S <filename>  set vdagent Unix domain socket [%s]\n"
> > > > -            "  -u <dev>       set uinput device       [%s]\n"
> > > > -            "  -f             treat uinput device as fake; no ioctls\n"
> > > > -            "  -x             don't daemonize\n"
> > > > -            "  -o             only handle one virtio serial session\n"
> > > > -#ifdef HAVE_CONSOLE_KIT
> > > > -            "  -X             disable console kit integration\n"
> > > > -#endif
> > > > -#ifdef HAVE_LIBSYSTEMD_LOGIN
> > > > -            "  -X             disable systemd-logind integration\n"
> > > > -#endif
> > > > -            ,VERSION, portdev, vdagentd_socket, uinput_device);
> > > > -}
> > > > -
> > > >  static void daemonize(void)
> > > >  {
> > > >      int x;
> > > > @@ -1081,52 +1064,80 @@ static void quit_handler(int sig)
> > > >      quit = 1;
> > > >  }
> > > >
> > > > +static gboolean parse_debug_level_cb(const gchar *option_name,
> > > > +                                     const gchar *value,
> > > > +                                     gpointer     data,
> > > > +                                     GError     **error)
> > > > +{
> > > > +    debug++;
> > > > +    return TRUE;
> > > > +}
> > >
> > > It is okay to leave this for now as this patches are touching the
> > > command line parsing part but within the context of glib
> > > integration, we might consider removing this in the future and
> > > follow what might be dictated by Glib's logging system.
> >
> > This parse function is necessary for the command line options
> > to stay the same.  The higher debug level is used in
> > vdagentd_uinput_create(..., debug > 1, ...).  So removing this
> > function would require adding a new option.  I would do this in
> > a separate patch.
>
> Yes, no need to do it now. It was more like a comment for future
> enhancements. This change would need glib's output to go to
> systemd journal fields by default and I think that's the case
> only with glib 2.50, but I could be wrong.
>
> > > > +static GOptionEntry cmd_entries[] = {
> > > > +    { "debug", 'd', G_OPTION_FLAG_NO_ARG,
> > > > +      G_OPTION_ARG_CALLBACK, parse_debug_level_cb,
> > > > +      "Log debug messages (use twice for extra info)", NULL },
> > > > +
> > > > +    { "virtio-serial-port-path", 's', 0,
> > > > +      G_OPTION_ARG_STRING, &portdev,
> > > > +      "Set virtio-serial path ("  DEFAULT_VIRTIO_PORT_PATH ")", NULL },
> > > > +
> > > > +    { "vdagentd-socket", 'S', 0,
> > > > +      G_OPTION_ARG_STRING, &vdagentd_socket,
> > > > +       "Set spice-vdagentd socket (" VDAGENTD_SOCKET ")", NULL },
> > > > +
> > > > +    { "uinput-device", 'u', 0,
> > > > +      G_OPTION_ARG_STRING, &uinput_device,
> > > > +      "Set uinput device (" DEFAULT_UINPUT_DEVICE ")", NULL },
> > > > +
> > > > +    { "fake-uinput", 'f', 0,
> > > > +      G_OPTION_ARG_NONE, &uinput_fake,
> > > > +      "Treat uinput device as fake; no ioctls", NULL },
> > > > +
> > > > +    { "foreground", 'x', G_OPTION_FLAG_REVERSE,
> > > > +      G_OPTION_ARG_NONE, &do_daemonize,
> > > > +      "Do not daemonize the agent", NULL},
> > > > +
> > > > +    { "one-session", 'o', 0,
> > > > +      G_OPTION_ARG_NONE, &only_once,
> > > > +      "Only handle one virtio serial session", NULL },
> > > > +
> > > > +#if defined(HAVE_CONSOLE_KIT) || defined (HAVE_LIBSYSTEMD_LOGIN)
> > > > +    { "disable-session-integration", 'X', G_OPTION_FLAG_REVERSE,
> > > > +      G_OPTION_ARG_NONE, &want_session_info,
> > > > +      "Disable console kit and systemd-logind integration", NULL },
> > > > +#endif
> > > > +
> > > > +    { NULL }
> > > > +};
> > > > +
> > > >  int main(int argc, char *argv[])
> > > >  {
> > > > -    int c;
> > > > -    int do_daemonize = 1;
> > > > -    int want_session_info = 1;
> > > > +    GOptionContext *context;
> > > > +    GError *err = NULL;
> > > >      struct sigaction act;
> > > >      gboolean own_socket = TRUE;
> > > >
> > > > -    for (;;) {
> > > > -        if (-1 == (c = getopt(argc, argv, "-dhxXfos:u:S:")))
> > > > -            break;
> > > > -        switch (c) {
> > > > -        case 'd':
> > > > -            debug++;
> > > > -            break;
> > > > -        case 's':
> > > > -            portdev = optarg;
> > > > -            break;
> > > > -        case 'S':
> > > > -            vdagentd_socket = optarg;
> > > > -            break;
> > > > -        case 'u':
> > > > -            uinput_device = optarg;
> > > > -            break;
> > > > -        case 'f':
> > > > -            uinput_fake = 1;
> > > > -            break;
> > > > -        case 'o':
> > > > -            only_once = 1;
> > > > -            break;
> > > > -        case 'x':
> > > > -            do_daemonize = 0;
> > > > -            break;
> > > > -        case 'X':
> > > > -            want_session_info = 0;
> > > > -            break;
> > > > -        case 'h':
> > > > -            usage(stdout);
> > > > -            return 0;
> > > > -        default:
> > > > -            fputs("\n", stderr);
> > > > -            usage(stderr);
> > > > -            return 1;
> > > > -        }
> > > > +    context = g_option_context_new(NULL);
> > > > +    g_option_context_add_main_entries(context, cmd_entries, NULL);
> > > > +    g_option_context_set_summary(context,
> > > > +        "Spice guest agent daemon, version " VERSION);
> > > > +    g_option_context_parse(context, &argc, &argv, &err);
> > > > +    g_option_context_free(context);
> > > > +
> > > > +    if (err) {
> > > > +        g_printerr("Invalid arguments, %s\n", err->message);
> > >
> > > We don't use g_printerr() or any g_log() in this code yet.
> >
> > I think I copied it from the vdagent.c where it was added in 0ffca2b
> > ("vdagent: Use glib's commandline parser").
> > I will change it to syslog().

One thing I did not realize while I was responding to you review yesterday:
If the vdagentd was started with wrong args, we probably want to
output the "Invalid arguments" message to the stderr. However,
syslog() does not always do that, it depends on the do_daemonize var
when we call openlog(). And the do_daemonize is set during the
parsing. If the parsing fails, do_daemonize might not be set
correctly. So using g_printerr() is correct in this case, IMHO.

Jakub
>
> Thanks,
> Victor
> > >
> > > I hope we can move to that whenever we bump glib to 2.50 or above
> > > as it writes to systemd's journal by default for daemons (AFAIK).
> > >
> > > Reviewed-by: Victor Toso <victortoso@redhat.com>
> > >
> > > Feel free to submit a new version with fixes as standalone patch
> > > with me in cc, to reduce the backlog of this series ;)
> > >
> > > Cheers,
> > > Victor
> > >
> > > > +        g_error_free(err);
> > > > +        return 1;
> > > >      }
> > > >
> > > > +    if (portdev == NULL)
> > > > +        portdev = g_strdup(DEFAULT_VIRTIO_PORT_PATH);
> > > > +    if (vdagentd_socket == NULL)
> > > > +        vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
> > > > +    if (uinput_device == NULL)
> > > > +        uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
> > > > +
> > > >      memset(&act, 0, sizeof(act));
> > > >      act.sa_flags = SA_RESTART;
> > > >      act.sa_handler = quit_handler;
> > > > @@ -1223,5 +1234,9 @@ int main(int argc, char *argv[])
> > > >      if (do_daemonize)
> > > >          unlink(pidfilename);
> > > >
> > > > +    g_free(portdev);
> > > > +    g_free(vdagentd_socket);
> > > > +    g_free(uinput_device);
> > > > +
> > > >      return retval;
> > > >  }
> > > > --
> > > > 2.17.1
> > > >
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> > Cheers,
> > Jakub
On Tue, Sep 04, 2018 at 02:44:29PM +0200, Jakub Janku wrote:
> > > > > +    if (err) {
> > > > > +        g_printerr("Invalid arguments, %s\n", err->message);
> > > >
> > > > We don't use g_printerr() or any g_log() in this code
> > > > yet.
> > >
> > > I think I copied it from the vdagent.c where it was added
> > > in 0ffca2b ("vdagent: Use glib's commandline parser").
> > > I will change it to syslog().
> 
> One thing I did not realize while I was responding to you
> review yesterday: If the vdagentd was started with wrong args,
> we probably want to output the "Invalid arguments" message to
> the stderr. However, syslog() does not always do that, it
> depends on the do_daemonize var when we call openlog().

Before this commit, usage() was doing fprintf() on stderr;
g_printerr() will do fputs() on stderr, so it should be fine.

> do_daemonize is set during the parsing. If the parsing fails,
> do_daemonize might not be set correctly. So using g_printerr()
> is correct in this case, IMHO.

Thanks,

> 
> Jakub
> >
> > Thanks,
> > Victor
> > > >
> > > > I hope we can move to that whenever we bump glib to 2.50 or above
> > > > as it writes to systemd's journal by default for daemons (AFAIK).
> > > >
> > > > Reviewed-by: Victor Toso <victortoso@redhat.com>
> > > >
> > > > Feel free to submit a new version with fixes as standalone patch
> > > > with me in cc, to reduce the backlog of this series ;)
> > > >
> > > > Cheers,
> > > > Victor
> > > >
> > > > > +        g_error_free(err);
> > > > > +        return 1;
> > > > >      }
> > > > >
> > > > > +    if (portdev == NULL)
> > > > > +        portdev = g_strdup(DEFAULT_VIRTIO_PORT_PATH);
> > > > > +    if (vdagentd_socket == NULL)
> > > > > +        vdagentd_socket = g_strdup(VDAGENTD_SOCKET);
> > > > > +    if (uinput_device == NULL)
> > > > > +        uinput_device = g_strdup(DEFAULT_UINPUT_DEVICE);
> > > > > +
> > > > >      memset(&act, 0, sizeof(act));
> > > > >      act.sa_flags = SA_RESTART;
> > > > >      act.sa_handler = quit_handler;
> > > > > @@ -1223,5 +1234,9 @@ int main(int argc, char *argv[])
> > > > >      if (do_daemonize)
> > > > >          unlink(pidfilename);
> > > > >
> > > > > +    g_free(portdev);
> > > > > +    g_free(vdagentd_socket);
> > > > > +    g_free(uinput_device);
> > > > > +
> > > > >      return retval;
> > > > >  }
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > > _______________________________________________
> > > > > Spice-devel mailing list
> > > > > Spice-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > > Cheers,
> > > Jakub