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@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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel