[v3,7/10] qemu-binfmt-conf.sh: add option --reset <ARCHS>

This is a partial implementation.

Allows to remove a single or a list of already registered binfmt
interpreters. If <ARCHS> is a list, it must be comma-separated.
Valid values are those in qemu_target_list. If <ARCHS> is 'ALL', all
the existing 'qemu-*' interpreters are removed.

This is partial because 'debian' and 'systemd' configurations are not
removed. If option 'reset' is provided before any of those, reset is
executed first and the configuration proceeds. However, if 'reset' is
provided after any of them, the script will exit with error 'option
reset not implemented for this mode yet'.

Removal is done by printing '-1' as explained at:
https://www.kernel.org/doc/Documentation/admin-guide/binfmt-misc.rst

Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>
---
 scripts/qemu-binfmt-conf.sh | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

--
2.20.1

On 06/03/2019 05:52, Unai Martinez-Corral wrote:
> This is a partial implementation.
> 
> Allows to remove a single or a list of already registered binfmt
> interpreters. If <ARCHS> is a list, it must be comma-separated.

I think ARCHS and CPUS are the same thing, so use the same word.

> Valid values are those in qemu_target_list. If <ARCHS> is 'ALL', all
> the existing 'qemu-*' interpreters are removed.
> 
> This is partial because 'debian' and 'systemd' configurations are not
> removed. If option 'reset' is provided before any of those, reset is
> executed first and the configuration proceeds. However, if 'reset' is
> provided after any of them, the script will exit with error 'option
> reset not implemented for this mode yet'.
> 
> Removal is done by printing '-1' as explained at:
> https://www.kernel.org/doc/Documentation/admin-guide/binfmt-misc.rst
> 
> Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>
> ---
>  scripts/qemu-binfmt-conf.sh | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
> index 2751363089..824e3c4c34 100755
> --- a/scripts/qemu-binfmt-conf.sh
> +++ b/scripts/qemu-binfmt-conf.sh
> @@ -197,8 +197,8 @@ qemu_get_family() {
> 
>  usage() {
>      cat <<EOF
> -Usage: qemu-binfmt-conf.sh [--path PATH][--debian][--systemd]
> -                           [--help][--credential][--exportdir PATH]
> +Usage: qemu-binfmt-conf.sh [--help][--path PATH][--debian][--systemd]
> +                           [--reset ARCHS][--credential][--exportdir PATH]
>                             [--persistent][--suffix SUFFIX][CPUS]

I don't like the idea of being able to set and reset different
interpretes with same command line call.

Make "--reset" exclusive without parameter to be able to use the common
cpu list at the end of the command line (this way, I think you would be
able to share more common code between setting an resetting).

You could have a look to Alex Bennée's patch:

  https://patchwork.ozlabs.org/patch/938796/

I think this is the good way to do this and it should merge easily with
your series.

Thanks,
Laurent

2019/3/10 18:15, Laurent Vivier:
> On 06/03/2019 05:52, Unai Martinez-Corral wrote:
> > Allows to remove a single or a list of already registered binfmt
> > interpreters. If <ARCHS> is a list, it must be comma-separated.
>
> I think ARCHS and CPUS are the same thing, so use the same word.

They are similar, but the syntax is not the same. ARCHS supports a
single word or a comma separated list. CPUS supports also using
spaces, tabs, etc. as separators.

> > -Usage: qemu-binfmt-conf.sh [--path PATH][--debian][--systemd]
> > -                           [--help][--credential][--exportdir PATH]
> > +Usage: qemu-binfmt-conf.sh [--help][--path PATH][--debian][--systemd]
> > +                           [--reset ARCHS][--credential][--exportdir PATH]
> >                             [--persistent][--suffix SUFFIX][CPUS]
>
> I don't like the idea of being able to set and reset different
> interpretes with same command line call.

> Make "--reset" exclusive without parameter to be able to use the common
> cpu list at the end of the command line

It is useful in order to replace a single interpreter or a few of them
in a single call to the script. Anyway, I think that your comment
makes sense. It is enough if we support to reset a subset. Any user
can then wrap two consecutive calls in a helper function.

So, in the next version '--reset' will be 'exclusive', i.e., it will
prevent registration.

> (this way, I think you would be
> able to share more common code between setting an resetting).

The shared code is the same, because 'qemu_check_target_list' was
already being used in both. The simplification comes from the fact
that we do not support commas anymore, since ARCHS and CPUS are now
only positional.

> You could have a look to Alex Bennée's patch:
>
>   https://patchwork.ozlabs.org/patch/938796/
>
> I think this is the good way to do this and it should merge easily with
> your series.

Thanks for the reference; I had skipped it. Upon further inspection, I
think that the features proposed by Alex are already implemented in
this patch, or they will be in the next version:

- I am already checking with '$CHECK', which will use the
corresponding procedure depending on 'systemd' or 'debian'.
- I am using printf instead of 'echo -1', as suggested by Eric Blake
in v1 of the patch.
- I will rename the option from '--reset' to '--clear', as I find it
to be more descriptive.
- None of us support neither '--debian' nor '--systemd'. See comments below.
- I support providing a subset of targets, while Alex' approach
targeted all of them.
- Overall, I think that 'BINFMT_SET=qemu_clear_interpreter' is ok for
a proof of concept, but it is not as clean as it could be. Precisely,
all the magic, mask, family values are computed in the loop inside
qemu_set_binfmts. That should be avoided if we only want to remove
them. Nonetheless, I will try to integrate both approaches.

Regarding this comment:

> I think it would be also useful to remove the files from $EXPORTDIR.
> So could you also manage something like "--debian --clear" and "--systemd CPU --clear"?

ATM, this patchset supports '--reset --debian' or '--reset --systemd',
but it will proceed by updating /proc directly, which might not be
effective at all.
Furthermore, when '--debian --reset' or '--systemd --reset' are used,
a error message is shown: 'ERROR: option reset not implemented for
this mode yet'.

On the one hand, I can make '--reset --debian|systemd' and
'--debian|systemd --reset' consistent, by delaying the execution of
the reset/clear function after all the options are parsed.

On the other hand, I'd really like to have patches 6 and 7 available
in QEMU 4.0, if possible. So, I'd like to know if the partial
implementation of this feature is a stopper. If so, I'd be so glad to
have some help. These are the prototypes that I have not added to the
patchset because I am not sure about them:

For the debian case, after the comments in 'usage()', I think that the
procedure should be:

for t in $qemu_check_target_list ; do
  sudo update-binfmts --package "qemu-$t" --remove "qemu-$t" "$QEMU_PATH"
  rm "$EXPORTDIR/qemu-$t"
done

And for systemd:

sudo systemctl stop systemd-binfmt.service
for t in $qemu_check_target_list ; do
   rm -rf $EXPORTDIR/qemu-$t.conf"
do
sudo systemctl start systemd-binfmt.service

Thanks,
Unai