Re: [Qemu-devel] [PATCH v7 3/9] qemu-binfmt-conf.sh: add QEMU_CREDENTIAL and QEMU_PERSISTENT

[Unai Martinez Corral]

2019/3/12 22:13, Eric Blake:
> On 3/12/19 2:51 PM, Unai Martinez-Corral wrote:
> > @@ -186,9 +186,11 @@ Usage: qemu-binfmt-conf.sh [--qemu-path 
> > PATH][--debian][--systemd CPU]
> >                        (default: $SYSTEMDDIR or $DEBIANDIR)
>
> Remember, this is in an unquoted heredoc, so it looks something like:
>
> (default: /etc/systemd or /usr/share/binfmts)
>
> when actually printed.

That's ok, isn't it? I thought about adding them to 'Defaults:' in
patch 4, but that can be misleading as Laurent pointed before (when I
added them to the Env-variables column). Also, I think that '(default:
SYSTEMDDIR=$SYSTEMDDIR or DEBIANDIR=$DEBIANDIR)' is not required,
because SYSTEMDDIR is likely to contain 'systemd'.

> >         --credential:  if present, credential and security tokens are
> >                        calculated according to the binary to interpret
> > +                      ($QEMU_CREDENTIAL=yes)
> >         --persistent:  if present, the interpreter is loaded when binfmt is
> >                        configured and remains in memory. All future uses
> >                        are cloned from the open file.
> > +                      ($QEMU_PERSISTENT=yes)
> >
>
> And with your change, when the environment starts empty (and thus the
> script's default initialization of QEMU_PERSISTENT kicks in), this will
> look like:
>
> (no=yes)
>
> You probably meant to write:
>
> (default: \$QEMU_PERSISTENT=no)
>
> to produce output of:
>
> (default: $QEMU_PERSISTENT=no)
>
> or:
>
> (default: \${QEMU_PERSISTENT:-no})
>
> to produce output of:
>
> (default: ${QEMU_PERSISTENT:-no})
>
> both to clarify that it defaults from the environment, and that its
> default is no rather than yes.

You are correct. I wanted the output to be 'QEMU_PERSISTENT=yes',
meaning that it is equivalent to '-p'. I did not mean to show the
default ('no'). Hope that the maintainer can remove the '$' if you are
ok with it, and we don't find other major issues.

Nonetheless, this is partially fixed in patch 4, where the format of
usage() is changed. There, defaults are properly shown in section
'Defaults'. However, I am not sure about the descriptions of '-p' and
'-c'. They start with '(yes)', which is the value the user should set
the envvars to. Do you suggest a different format to better explain
it?