This patch breaks backward compatibility.
Both '--persistent' and '--credential' default to 'no'. Hence, '-p no'
or '-c no' are reduntant. Overall, accepting an argument might be
misleading because options are, indeed, boolean. This patch makes both
options boolean in getopt, so if any of them is provided the corresponding
variable is set to true.
Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
scripts/qemu-binfmt-conf.sh | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 0009385be2..ca15ff8092 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -168,8 +168,8 @@ qemu_get_family() {
usage() {
cat <<EOF
Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
- [--help][--credential yes|no][--exportdir PATH]
- [--persistent yes|no][--qemu-suffix SUFFIX]
+ [--help][--credential][--exportdir PATH]
+ [--persistent][--qemu-suffix SUFFIX]
Configure binfmt_misc to use qemu interpreter
@@ -184,9 +184,9 @@ Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
file for all known cpus
--exportdir: define where to write configuration files
(default: $SYSTEMDDIR or $DEBIANDIR)
- --credential: if yes, credential and security tokens are
+ --credential: if present, credential and security tokens are
calculated according to the binary to interpret
- --persistent: if yes, the interpreter is loaded when binfmt is
+ --persistent: if present, the interpreter is loaded when binfmt is
configured and remains in memory. All future uses
are cloned from the open file.
@@ -324,7 +324,7 @@ CREDENTIAL=no
PERSISTENT=no
QEMU_SUFFIX=""
-options=$(getopt -o ds:Q:S:e:hc:p: -l debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential:,persistent: -- "$@")
+options=$(getopt -o ds:Q:S:e:hcp -l debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential,persistent -- "$@")
eval set -- "$options"
while true ; do
@@ -373,12 +373,10 @@ while true ; do
exit 1
;;
-c|--credential)
- shift
- CREDENTIAL="$1"
+ CREDENTIAL="yes"
;;
-p|--persistent)
- shift
- PERSISTENT="$1"
+ PERSISTENT="yes"
;;
*)
break
--
2.21.0
On 3/12/19 2:51 PM, Unai Martinez-Corral wrote: > This patch breaks backward compatibility. > Is it worth a mention why we don't consider backwards-compatibility for this script to be very important? > Both '--persistent' and '--credential' default to 'no'. Hence, '-p no' > or '-c no' are reduntant. Overall, accepting an argument might be redundant (Maintainer could fix up the typo without you needing to send v8, if the only thing I find are minor like that) > misleading because options are, indeed, boolean. This patch makes both > options boolean in getopt, so if any of them is provided the corresponding > variable is set to true. > > Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus> > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > --- > scripts/qemu-binfmt-conf.sh | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
2019/3/12, Eric Blake: > > On 3/12/19 2:51 PM, Unai Martinez-Corral wrote: > > This patch breaks backward compatibility. > > > > Is it worth a mention why we don't consider backwards-compatibility for > this script to be very important? 'persistent' is a quite recent feature (8 monts) which seems not to be widely know. See, e.g., the references in https://github.com/umarcor/qus#similar-projects-blog-posts-and-other-references. So I think that it is almost harmless. Regarding 'credential', I don't know. > > Both '--persistent' and '--credential' default to 'no'. Hence, '-p no' > > or '-c no' are reduntant. Overall, accepting an argument might be > > redundant > > (Maintainer could fix up the typo without you needing to send v8, if the > only thing I find are minor like that) Good catch.
On 3/12/19 4:39 PM, Unai Martinez Corral wrote: > 2019/3/12, Eric Blake: >> >> On 3/12/19 2:51 PM, Unai Martinez-Corral wrote: >>> This patch breaks backward compatibility. >>> >> >> Is it worth a mention why we don't consider backwards-compatibility for >> this script to be very important? > > 'persistent' is a quite recent feature (8 monts) which seems not to be > widely know. See, e.g., the references in > https://github.com/umarcor/qus#similar-projects-blog-posts-and-other-references. This rationale belongs in the commit message. > So I think that it is almost harmless. I don't know the prospective audience of qemu-binfmt-conf.sh to know if it is harmless or not. I'm just trying to make sure that if your commit DOES end up breaking something, that whoever ends up bisecting back to your commit message has enough information to know whether they need to fix their workflow or propose a patch to revert your change. I also don't know if this is the sort of change that should go through the formal qemu-deprecation.texi policy of two cycles of warnings about pending change in behavior before we actually flip the switch, or if it is indeed a small enough audience that a formal deprecation is too much effort. Backward-incompatible changes are not necessarily wrong, but they DESERVE some specific attention in the commit message so that people reading the commit can have warm fuzzies about the change. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
2019/3/12 22:56, Eric Blake: > On 3/12/19 4:39 PM, Unai Martinez Corral wrote: > > 2019/3/12, Eric Blake: > >> > >> On 3/12/19 2:51 PM, Unai Martinez-Corral wrote: > >>> This patch breaks backward compatibility. > >> > >> Is it worth a mention why we don't consider backwards-compatibility for > >> this script to be very important? > > > > 'persistent' is a quite recent feature (8 monts) which seems not to be > > widely know. See, e.g., the references in > > https://github.com/umarcor/qus#similar-projects-blog-posts-and-other-references. > > This rationale belongs in the commit message. But there is no rationale here. It is just a style preference, which is explained in the commit message. Should I add a comment starting with 'I feel' or 'as far as I am aware' to let others know that there is no strong argument behind the decission? > > So I think that it is almost harmless. > > I don't know the prospective audience of qemu-binfmt-conf.sh to know if > it is harmless or not. I'm just trying to make sure that if your commit > DOES end up breaking something, that whoever ends up bisecting back to > your commit message has enough information to know whether they need to > fix their workflow or propose a patch to revert your change. It is quiet possible that these changes will end up breaking things. Not only these, but also those in patches 5 and 7. In none of them have I explained why is backwards-compatibility not important. Better said, less important than the overall improvement of this patchset. Since explanations underline the motivation for the changes, I think that it is implicit why backward compatibility is deemed less important. Moreover, since all of them are pushed together, I hope that whoever bisecting back will find that it is a rewrite to improve consistency; not just an random atomic change. > I also don't know if this is the sort of change that should go through the > formal qemu-deprecation.texi policy of two cycles of warnings about > pending change in behavior before we actually flip the switch, or if it > is indeed a small enough audience that a formal deprecation is too much > effort. I didn't know that the formal deprecation policy applied to changes. I assumed that maintainers would add these to https://wiki.qemu.org/ChangeLog/4.0#Incompatible_changes. I am checking it anyway. Regarding the two cycles of warnings, since we cannot support both formats at the same time, does it mean that the patchset should be divided to two? I.e., one with the fixes/features that can be added now and a bunch of warnings; and a second one to be merged after two cycles?
© 2016 - 2025 Red Hat, Inc.