Re: [Qemu-devel] [PATCH v3 7/10] qemu-binfmt-conf.sh: add option --reset <ARCHS>

[Unai Martinez Corral]

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