Re: [libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
On Mon, 2019-07-29 at 10:51 +0100, Daniel P. Berrangé wrote:
> 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:
> > > +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
>
> You're only thinking about this from POV of english text. When this
> is translated into other languages, all bets are off for such
> alignment, as translated text could easily be longer than 18 characters
> and I don't fancy guessing the max field width to cope with arbitrary
> languages.
Right, I was tricked by the fact that you use "%-22s" earlier in the
same function, but of course that's appropriate because you're
printing the option names and those are not subject to i18n...
We should move the vshTable API inside libvirt so that we can do this
kind of thing easily! (Only partially kidding.)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
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 ", N_("Exit after timeout period") },
> > +{ "-f | --config ", N_("Configuration file") },
> > +{ "-V | --version", N_("Display version information") },
> > +{ "-p | --pid-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
You're only thinking about this from POV of english text. When this
is translated into other languages, all bets are off for such
alignment, as translated text could easily be longer than 18 characters
and I don't fancy guessing the max field width to cope with arbitrary
languages.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
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 ", N_("Exit after timeout period") },
> > +{ "-f | --config ", N_("Configuration file") },
> > +{ "-V | --version", N_("Display version information") },
> > +{ "-p | --pid-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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
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 ", N_("Exit after timeout period") },
> +{ "-f | --config ", N_("Configuration file") },
> +{ "-V | --version", N_("Display version information") },
> +{ "-p | --pid-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.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 06/41] remote: stop trying to print help as giant blocks of text
The remote daemon tries to print out its help text in a couple of giant
blocks of text. This has already lead to duplication of the text for the
privileged vs unprivileged execution mode. With the introduction of more
daemons, this text is going to be duplicated many more times with small
variations. This is very unfriendly to translators as they have to
translate approximately the same text many times with small tweaks.
Splitting the text up into individual strings to print means that each
piece will only need translating once. It also gets rid of all the
layout information from the translated strings, so avoids the problem of
translators breaking formatting by mistake.
Signed-off-by: Daniel P. Berrangé
---
src/remote/remote_daemon.c | 126 ++---
1 file changed, 62 insertions(+), 64 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index d887b7abfb..49ac3f0ae5 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -863,71 +863,69 @@ daemonSetupHostUUID(const struct daemonConfig *config)
static void
daemonUsage(const char *argv0, bool privileged)
{
-fprintf(stderr,
-_("\n"
- "Usage:\n"
- " %s [options]\n"
- "\n"
- "Options:\n"
- " -h | --helpDisplay program help:\n"
- " -v | --verbose Verbose messages.\n"
- " -d | --daemon Run as a daemon & write PID file.\n"
- " -l | --listen Listen for TCP/IP connections.\n"
- " -t | --timeout Exit after timeout period.\n"
- " -f | --configConfiguration file.\n"
- " -V | --version Display version information.\n"
- " -p | --pid-file Change name of PID file.\n"
- "\n"
- "libvirt management daemon:\n"),
-argv0);
+size_t i;
+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 ", N_("Exit after timeout period") },
+{ "-f | --config ", N_("Configuration file") },
+{ "-V | --version", N_("Display version information") },
+{ "-p | --pid-file ", N_("Change name of PID file") },
+};
-if (privileged) {
-fprintf(stderr,
-_("\n"
- " Default paths:\n"
- "\n"
- "Configuration file (unless overridden by -f):\n"
- " %s\n"
- "\n"
- "Sockets:\n"
- " %s\n"
- " %s\n"
- "\n"
- "TLS:\n"
- " CA certificate: %s\n"
- " Server certificate: %s\n"
- " Server private key: %s\n"
- "\n"
- "PID file (unless overridden by -p):\n"
- " %s/run/libvirtd.pid\n"
- "\n"),
-LIBVIRTD_CONFIGURATION_FILE,
-LIBVIRTD_PRIV_UNIX_SOCKET,
-LIBVIRTD_PRIV_UNIX_SOCKET_RO,
-LIBVIRT_CACERT,
-LIBVIRT_SERVERCERT,
-LIBVIRT_SERVERKEY,
-LOCALSTATEDIR);
-} else {
-fprintf(stderr, "%s",
-_("\n"
- " Default paths:\n"
- "\n"
- "Configuration file (unless overridden by -f):\n"
- " $XDG_CONFIG_HOME/libvirt/libvirtd.conf\n"
- "\n"
- "Sockets:\n"
- " $XDG_RUNTIME_DIR/libvirt/libvirt-sock\n"
- "\n"
- "TLS:\n"
- " CA certificate: $HOME/.pki/libvirt/cacert.pem\n"
- " Server certificate:
$HOME/.pki/libvirt/servercert.pem\n"
- " Server private key:
$HOME/.pki/libvirt/serverkey.pem\n"
- "\n"
- "PID file:\n"
- " $XDG_RUNTIME_DIR/libvirt/libvirtd.pid\n"
- "\n"));
-}
+fprintf(stderr, "\n");
+fprintf(stderr, "%s:\n", _("Usage"));
+fprintf(stderr, " %s [%s]\n", argv0, _("options"));
+fprintf(stderr, "\n");
+
+fprintf(stderr, "%s:\n", _("Options"));
+for (i = 0; i < ARRAY_CARDINALITY(opthelp); i++)
+fprintf(stderr, " %-22s %s\n", opthelp[i].opts, N_(opthelp[i].help));
+fprintf(stderr, "\n");
+
+fprintf(stderr, "%s:\n", _("libvirt management daemon"));
+
+fprintf(stderr, "\n");
+fprintf(stderr, " %s:\n", _("Default paths"));
+fprintf(stderr, "\n");
+
+fprint
