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
