[libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text

[Daniel P. Berrangé]

On Fri, Jul 26, 2019 at 02:02:18PM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
> [...]
> > +    struct virOptionHelp {
> > +        const char *opts;
> > +        const char *help;
> > +    } opthelp[] = {
> > +        { "-h | --help", N_("Display program help") },
> > +        { "-v | --verbose", N_("Verbose messages") },
> > +        { "-d | --daemon", N_("Run as a daemon & write PID file") },
> > +        { "-l | --listen", N_("Listen for TCP/IP connections") },
> > +        { "-t | --timeout <secs>", N_("Exit after timeout period") },
> > +        { "-f | --config <file>", N_("Configuration file") },
> > +        { "-V | --version", N_("Display version information") },
> > +        { "-p | --pid-file <file>", N_("Change name of PID file") },
> > +    };
> 
> The way you declare the struct at the same time as you use it for a
> local variable is highly unusual in our codebase, especially
> considering that you do the former inside a function which is, at
> least as far as I'm aware, basically unprecedented in libvirt.
> 
> Can you please move the struct declaration outside of the function?
> 
> [...]
> > +    fprintf(stderr, "    %s:\n", _("TLS"));
> > +    fprintf(stderr, "      %s: %s\n",
> > +            _("CA certificate"),
> > +            privileged ? LIBVIRT_CACERT : "$HOME/.pki/libvirt/cacert.pem");
> > +    fprintf(stderr, "      %s: %s\n",
> > +            _("Server certificate"),
> > +            privileged ? LIBVIRT_SERVERCERT : "$HOME/.pki/libvirt/servercert.pem");
> > +    fprintf(stderr, "      %s: %s\n",
> > +            _("Server private key"),
> > +            privileged ? LIBVIRT_SERVERKEY : "$HOME/.pki/libvirt/serverkey.pem");
> > +    fprintf(stderr, "\n");
> 
> I think the above would work better if you used
> 
>   "      %-18s  %s\n"
> 
> as the format string, which would result in
> 
>   TLS:
>     CA certificate      $HOME/.pki/libvirt/cacert.pem
>     Server certificate  $HOME/.pki/libvirt/servercert.pem
>     Server private key  $HOME/.pki/libvirt/serverkey.pem
> 
> instead of
> 
>   TLS:
>     CA certificate: $HOME/.pki/libvirt/cacert.pem
>     Server certificate: $HOME/.pki/libvirt/servercert.pem
>     Server private key: $HOME/.pki/libvirt/serverkey.pem
> 
> as the output. Not a big deal either way, but since we're going out
> of our way to align options and their descriptions above it makes
> sense to me that we'd do the same here as well.
> 
> 
> After this patch, the code looks much less readable, but I understand
> why you want to do this and you also get rid of some duplication when
> it comes to the system/session daemons, so I'm overall okay with the
> change.

Yeah it is pretty ugly, but at least some of this uglyness goes away
if we adopt GLib, because its CLI parsing APIs are much better than
getopt, in particular capable of auto-generating the help output for
all args.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|