| Message ID | 20190312195140.GC8@99bbefa4bcea (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-binfmt-conf.sh | expand |
On 3/12/19 2:51 PM, Unai Martinez-Corral wrote: > Allow to set options '--persistent' and/or '--credential' through > environment variables. If not defined, defaults are used ('no'). > Anyway, command-line arguments have priority over environment variables. > > Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus> > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > --- > scripts/qemu-binfmt-conf.sh | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh > index ca15ff8092..ad9ae731a0 100755 > --- a/scripts/qemu-binfmt-conf.sh > +++ b/scripts/qemu-binfmt-conf.sh > @@ -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. > --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. > + > +QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}" > +QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}" > + > QEMU_SUFFIX="" >
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?
diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh index ca15ff8092..ad9ae731a0 100755 --- a/scripts/qemu-binfmt-conf.sh +++ b/scripts/qemu-binfmt-conf.sh @@ -186,9 +186,11 @@ Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU] (default: $SYSTEMDDIR or $DEBIANDIR) --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) To import templates with update-binfmts, use : @@ -255,10 +257,10 @@ qemu_check_systemd() { qemu_generate_register() { flags="" - if [ "x$CREDENTIAL" = "xyes" ] ; then + if [ "x$QEMU_CREDENTIAL" = "xyes" ] ; then flags="OC" fi - if [ "x$PERSISTENT" = "xyes" ] ; then + if [ "x$QEMU_PERSISTENT" = "xyes" ] ; then flags="${flags}F" fi @@ -281,7 +283,7 @@ package qemu-$cpu interpreter $qemu magic $magic mask $mask -credential $CREDENTIAL +credential $QEMU_CREDENTIAL EOF } @@ -320,8 +322,10 @@ SYSTEMDDIR="/etc/binfmt.d" DEBIANDIR="/usr/share/binfmts" QEMU_PATH=/usr/local/bin -CREDENTIAL=no -PERSISTENT=no + +QEMU_PERSISTENT="${QEMU_PERSISTENT:-no}" +QEMU_CREDENTIAL="${QEMU_CREDENTIAL:-no}" + QEMU_SUFFIX="" options=$(getopt -o ds:Q:S:e:hcp -l debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential,persistent -- "$@") @@ -373,10 +377,10 @@ while true ; do exit 1 ;; -c|--credential) - CREDENTIAL="yes" + QEMU_CREDENTIAL="yes" ;; -p|--persistent) - PERSISTENT="yes" + QEMU_PERSISTENT="yes" ;; *) break
Allow to set options '--persistent' and/or '--credential' through environment variables. If not defined, defaults are used ('no'). Anyway, command-line arguments have priority over environment variables. Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus> Reviewed-by: Laurent Vivier <laurent@vivier.eu> --- scripts/qemu-binfmt-conf.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) -- 2.21.0