[Spice-devel] [RFC spice-vdagent 01/18] vdagentd: parse argv using GLib
Jakub Janku
jjanku at redhat.com
Mon Sep 3 16:06:01 UTC 2018
Hi,
On Tue, Aug 28, 2018 at 7:16 AM Victor Toso <victortoso at 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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Cheers,
Jakub
More information about the Spice-devel
mailing list