[PATCH v8 1/9] qemu-binfmt-conf.sh: enforce safe style consistency

[PATCH v8 1/9] qemu-binfmt-conf.sh: enforce safe style consistency

[Unai Martinez-Corral]

Spaces are added before '; then', for consistency.

All the tests are prefixed with 'x', in order to avoid risky comparisons
(i.e. a user deliberately trying to provoke a syntax error).

Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 scripts/qemu-binfmt-conf.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 9f1580a91c..672ce716b6 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -223,12 +223,12 @@ qemu_check_access() {
 
 qemu_check_bintfmt_misc() {
     # load the binfmt_misc module
-    if [ ! -d /proc/sys/fs/binfmt_misc ]; then
+    if [ ! -d /proc/sys/fs/binfmt_misc ] ; then
       if ! /sbin/modprobe binfmt_misc ; then
           exit 1
       fi
     fi
-    if [ ! -f /proc/sys/fs/binfmt_misc/register ]; then
+    if [ ! -f /proc/sys/fs/binfmt_misc/register ] ; then
       if ! mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc ; then
           exit 1
       fi
@@ -259,10 +259,10 @@ qemu_check_systemd() {
 
 qemu_generate_register() {
     flags=""
-    if [ "$CREDENTIAL" = "yes" ] ; then
+    if [ "x$CREDENTIAL" = "xyes" ] ; then
         flags="OC"
     fi
-    if [ "$PERSISTENT" = "yes" ] ; then
+    if [ "x$PERSISTENT" = "xyes" ] ; then
         flags="${flags}F"
     fi
 
@@ -300,18 +300,18 @@ qemu_set_binfmts() {
         mask=$(eval echo \$${cpu}_mask)
         family=$(eval echo \$${cpu}_family)
 
-        if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then
+        if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family" = "x" ] ; then
             echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2
             continue
         fi
 
         qemu="$QEMU_PATH/qemu-$cpu"
-        if [ "$cpu" = "i486" ] ; then
+        if [ "x$cpu" = "xi486" ] ; then
             qemu="$QEMU_PATH/qemu-i386"
         fi
 
         qemu="$qemu$QEMU_SUFFIX"
-        if [ "$host_family" != "$family" ] ; then
+        if [ "x$host_family" != "x$family" ] ; then
             $BINFMT_SET
         fi
     done
-- 
2.25.1

Re: [PATCH v8 1/9] qemu-binfmt-conf.sh: enforce safe style consistency

[Eric Blake]

On 3/7/20 11:22 AM, Unai Martinez-Corral wrote:
> Spaces are added before '; then', for consistency.

For consistency with what?  This is not our prevailing style; as 
evidenced by this pre-patch search:

$ git grep 'if \[.*\];' | wc
     274    2186   18170
$ git grep 'if \[.*\] ;' | wc
      25     256    1573

and you are diverging from the dominant pattern.

> 
> All the tests are prefixed with 'x', in order to avoid risky comparisons
> (i.e. a user deliberately trying to provoke a syntax error).

This part, however, is good.  Since one part is controversial, you may 
want to split this into two patches, or even drop the reformatting part.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v8 1/9] qemu-binfmt-conf.sh: enforce safe style consistency

[Unai Martinez Corral]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

 2020/03/09 16:01, Eric Blake:

> On 3/7/20 11:22 AM, Unai Martinez-Corral wrote:
> > Spaces are added before '; then', for consistency.
>
> For consistency with what?  This is not our prevailing style; as
> evidenced by this pre-patch search:
>
> $ git grep 'if \[.*\];' | wc
>      274    2186   18170
> $ git grep 'if \[.*\] ;' | wc
>       25     256    1573
>
> and you are diverging from the dominant pattern.
>

For consistency within the script that is being modified. I'm not trying to
diverge, neither do I prefer any specific style.
Although the style in the current master is not consistent, ' ; ' is
significantly more frequent. When I was told to keep consistency in v2, I
picked that because it was the most common.
Anyway, I will push a new version where all these are changed to the
dominant pattern outside of this script.


> This part, however, is good.  Since one part is controversial, you may
> want to split this into two patches, or even drop the reformatting part.
>

Since the current master is neither consistent nor coherent with the
dominant pattern, I don't think I can drop the reformatting as long as I
want to fulfill your requirements.

[-- Attachment #2: Type: text/html, Size: 1775 bytes --]

Re: [PATCH v8 1/9] qemu-binfmt-conf.sh: enforce safe style consistency

[Eric Blake]

On 3/9/20 1:20 PM, Unai Martinez Corral wrote:
>   2020/03/09 16:01, Eric Blake:
> 
>> On 3/7/20 11:22 AM, Unai Martinez-Corral wrote:
>>> Spaces are added before '; then', for consistency.
>>
>> For consistency with what?  This is not our prevailing style; as
>> evidenced by this pre-patch search:
>>
>> $ git grep 'if \[.*\];' | wc
>>       274    2186   18170
>> $ git grep 'if \[.*\] ;' | wc
>>        25     256    1573
>>
>> and you are diverging from the dominant pattern.
>>
> 
> For consistency within the script that is being modified. I'm not trying to
> diverge, neither do I prefer any specific style.

Aha, I see what you were looking at: within the script itself, it was 10 
'] ;' vs. 2 '];'.  In which case, I'd recommend swapping the 10 
instances over to be common with the rest of the code base, rather than 
the 2 away from the rest of the code base but towards the rest of the 
script.

> Although the style in the current master is not consistent, ' ; ' is
> significantly more frequent. When I was told to keep consistency in v2, I
> picked that because it was the most common.
> Anyway, I will push a new version where all these are changed to the
> dominant pattern outside of this script.

Good to hear.

> 
> 
>> This part, however, is good.  Since one part is controversial, you may
>> want to split this into two patches, or even drop the reformatting part.
>>
> 
> Since the current master is neither consistent nor coherent with the
> dominant pattern, I don't think I can drop the reformatting as long as I
> want to fulfill your requirements.

Splitting into two patches (one to fix '] ;' spacing, the other to add 
'[ "x$..."' protection) is then the best course of action.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v8 1/9] qemu-binfmt-conf.sh: enforce safe style consistency

[Unai Martinez-Corral]

Spaces are added before '; then', for consistency.

All the tests are prefixed with 'x', in order to avoid risky comparisons
(i.e. a user deliberately trying to provoke a syntax error).

Signed-off-by: Unai Martinez-Corral <unai.martinezcorral@ehu.eus>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 scripts/qemu-binfmt-conf.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index 9f1580a91c..672ce716b6 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -223,12 +223,12 @@ qemu_check_access() {
 
 qemu_check_bintfmt_misc() {
     # load the binfmt_misc module
-    if [ ! -d /proc/sys/fs/binfmt_misc ]; then
+    if [ ! -d /proc/sys/fs/binfmt_misc ] ; then
       if ! /sbin/modprobe binfmt_misc ; then
           exit 1
       fi
     fi
-    if [ ! -f /proc/sys/fs/binfmt_misc/register ]; then
+    if [ ! -f /proc/sys/fs/binfmt_misc/register ] ; then
       if ! mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc ; then
           exit 1
       fi
@@ -259,10 +259,10 @@ qemu_check_systemd() {
 
 qemu_generate_register() {
     flags=""
-    if [ "$CREDENTIAL" = "yes" ] ; then
+    if [ "x$CREDENTIAL" = "xyes" ] ; then
         flags="OC"
     fi
-    if [ "$PERSISTENT" = "yes" ] ; then
+    if [ "x$PERSISTENT" = "xyes" ] ; then
         flags="${flags}F"
     fi
 
@@ -300,18 +300,18 @@ qemu_set_binfmts() {
         mask=$(eval echo \$${cpu}_mask)
         family=$(eval echo \$${cpu}_family)
 
-        if [ "$magic" = "" ] || [ "$mask" = "" ] || [ "$family" = "" ] ; then
+        if [ "x$magic" = "x" ] || [ "x$mask" = "x" ] || [ "x$family" = "x" ] ; then
             echo "INTERNAL ERROR: unknown cpu $cpu" 1>&2
             continue
         fi
 
         qemu="$QEMU_PATH/qemu-$cpu"
-        if [ "$cpu" = "i486" ] ; then
+        if [ "x$cpu" = "xi486" ] ; then
             qemu="$QEMU_PATH/qemu-i386"
         fi
 
         qemu="$qemu$QEMU_SUFFIX"
-        if [ "$host_family" != "$family" ] ; then
+        if [ "x$host_family" != "x$family" ] ; then
             $BINFMT_SET
         fi
     done
-- 
2.25.1