On Tue, Sep 4, 2018 at 7:02 AM Victor Toso <victortoso@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> > Cheers,
> > Jakub
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel