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

[Victor Toso]

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 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180904/b305374b/attachment.sig>