[PATCH v2 0/4] iotests: trivial cleanups

[PATCH v2 0/4] iotests: trivial cleanups

[Cleber Rosa]

The most trivial set of cleanups to iotests common libraries and the
044 test.

Changes from v1:
 * Added emacs mode hints on tests/qemu-iotests/common.* files (Eric
   Blake)

Cleber Rosa (4):
  qemu-iotests: remove bash shebang from library files
  qemu-iotests: remove forceful execution success from library files
  qemu-iotests: 044: pass is actually a noop, so remove it
  qemu-iotests: 044: remove inaccurate docstring class description

 tests/qemu-iotests/044            | 4 ----
 tests/qemu-iotests/common.config  | 5 +----
 tests/qemu-iotests/common.filter  | 5 +----
 tests/qemu-iotests/common.nbd     | 3 +--
 tests/qemu-iotests/common.pattern | 2 +-
 tests/qemu-iotests/common.qemu    | 2 +-
 tests/qemu-iotests/common.rc      | 5 +----
 tests/qemu-iotests/common.tls     | 2 +-
 8 files changed, 7 insertions(+), 21 deletions(-)

-- 
2.21.0

[PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Cleber Rosa]

Due to not being able to find a reason to have shebangs on files that
are not executable.

While at it, add a mode hint to emacs, which would be clueless or
plain wrong about these containing shell code.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/qemu-iotests/common.config  | 2 +-
 tests/qemu-iotests/common.filter  | 2 +-
 tests/qemu-iotests/common.nbd     | 3 +--
 tests/qemu-iotests/common.pattern | 2 +-
 tests/qemu-iotests/common.qemu    | 2 +-
 tests/qemu-iotests/common.rc      | 2 +-
 tests/qemu-iotests/common.tls     | 2 +-
 7 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 9bd1a5a6fc..b85a6a6f96 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+# -*- emacs mode: sh -*-
 #
 # Copyright (C) 2009 Red Hat, Inc.
 # Copyright (c) 2000-2003,2006 Silicon Graphics, Inc.  All Rights Reserved.
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 9f418b4881..a0b52b0ac8 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+# -*- emacs mode: sh -*-
 #
 # Copyright (C) 2009 Red Hat, Inc.
 # Copyright (c) 2000-2001 Silicon Graphics, Inc.  All Rights Reserved.
diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 24b01b60aa..a98d05d404 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -1,5 +1,4 @@
-#!/usr/bin/env bash
-# -*- shell-script-mode -*-
+# -*- emacs mode: sh -*-
 #
 # Helpers for NBD server related config
 #
diff --git a/tests/qemu-iotests/common.pattern b/tests/qemu-iotests/common.pattern
index 4f5e5bcea0..4028b32c54 100644
--- a/tests/qemu-iotests/common.pattern
+++ b/tests/qemu-iotests/common.pattern
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+# -*- emacs mode: sh -*-
 #
 # Copyright (C) 2009 Red Hat, Inc.
 #
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 8d2021a7eb..66569f6bdd 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+# -*- emacs mode: sh -*-
 #
 # This allows for launching of multiple QEMU instances, with independent
 # communication possible to each instance.
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index e45cdfa66b..aa4a7fcc11 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+# -*- emacs mode: sh -*-
 #
 # Copyright (C) 2009 Red Hat, Inc.
 # Copyright (c) 2000-2006 Silicon Graphics, Inc.  All Rights Reserved.
diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
index 54c331d7a5..759a3d6d71 100644
--- a/tests/qemu-iotests/common.tls
+++ b/tests/qemu-iotests/common.tls
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+# -*- emacs mode: sh -*-
 #
 # Helpers for TLS related config
 #
-- 
2.21.0

[PATCH v2 2/4] qemu-iotests: remove forceful execution success from library files

[Cleber Rosa]

Should not be necessary on files that are not executed standalone.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/qemu-iotests/common.config | 3 ---
 tests/qemu-iotests/common.filter | 3 ---
 tests/qemu-iotests/common.rc     | 3 ---
 3 files changed, 9 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index b85a6a6f96..47605d61e2 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -36,6 +36,3 @@ _optstr_add()
         echo "$2"
     fi
 }
-
-# make sure this script returns success
-true
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index a0b52b0ac8..60f675251a 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -226,6 +226,3 @@ _filter_qmp_empty_return()
 {
     grep -v '{"return": {}}'
 }
-
-# make sure this script returns success
-true
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index aa4a7fcc11..3fd4330dbd 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -602,6 +602,3 @@ _require_drivers()
         fi
     done
 }
-
-# make sure this script returns success
-true
-- 
2.21.0

[PATCH v2 3/4] qemu-iotests: 044: pass is actually a noop, so remove it

[Cleber Rosa]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/qemu-iotests/044 | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 8b2afa2a11..aa2a00ceed 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -102,17 +102,14 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=512', test_img, '16G')
         self.preallocate(test_img)
-        pass
 
 
     def tearDown(self):
         os.remove(test_img)
-        pass
 
     def test_grow_refcount_table(self):
         qemu_io('-c', 'write 3800M 1M', test_img)
         qemu_img_verbose('check' , test_img)
-        pass
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'],
-- 
2.21.0

[PATCH v2 4/4] qemu-iotests: 044: remove inaccurate docstring class description

[Cleber Rosa]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/qemu-iotests/044 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index aa2a00ceed..bae99e25cf 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -31,7 +31,6 @@ import sys
 test_img = os.path.join(iotests.test_dir, 'test.img')
 
 class TestRefcountTableGrowth(iotests.QMPTestCase):
-    '''Abstract base class for image mirroring test cases'''
 
     def preallocate(self, name):
         fd = open(name, "r+b")
-- 
2.21.0

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Eric Blake]

On 10/9/19 2:47 PM, Cleber Rosa wrote:
> Due to not being able to find a reason to have shebangs on files that
> are not executable.
> 
> While at it, add a mode hint to emacs, which would be clueless or
> plain wrong about these containing shell code.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/qemu-iotests/common.config  | 2 +-
>   tests/qemu-iotests/common.filter  | 2 +-
>   tests/qemu-iotests/common.nbd     | 3 +--
>   tests/qemu-iotests/common.pattern | 2 +-
>   tests/qemu-iotests/common.qemu    | 2 +-
>   tests/qemu-iotests/common.rc      | 2 +-
>   tests/qemu-iotests/common.tls     | 2 +-
>   7 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> index 9bd1a5a6fc..b85a6a6f96 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +# -*- emacs mode: sh -*-

I thought my version:
# hey emacs, this file will be sourced by bash -*- mode: sh -*-
was cuter, but that's not a requirement, and yours works  ;)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Cleber Rosa]

----- Original Message -----
> From: "Eric Blake" <eblake@redhat.com>
> To: "Cleber Rosa" <crosa@redhat.com>, qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org, "Max Reitz" <mreitz@redhat.com>, "Kevin Wolf" <kwolf@redhat.com>, qemu-trivial@nongnu.org,
> "Michael Tokarev" <mjt@tls.msk.ru>, "Laurent Vivier" <laurent@vivier.eu>
> Sent: Wednesday, October 9, 2019 3:51:56 PM
> Subject: Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files
> 
> On 10/9/19 2:47 PM, Cleber Rosa wrote:
> > Due to not being able to find a reason to have shebangs on files that
> > are not executable.
> > 
> > While at it, add a mode hint to emacs, which would be clueless or
> > plain wrong about these containing shell code.
> > 
> > Suggested-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/qemu-iotests/common.config  | 2 +-
> >   tests/qemu-iotests/common.filter  | 2 +-
> >   tests/qemu-iotests/common.nbd     | 3 +--
> >   tests/qemu-iotests/common.pattern | 2 +-
> >   tests/qemu-iotests/common.qemu    | 2 +-
> >   tests/qemu-iotests/common.rc      | 2 +-
> >   tests/qemu-iotests/common.tls     | 2 +-
> >   7 files changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/common.config
> > b/tests/qemu-iotests/common.config
> > index 9bd1a5a6fc..b85a6a6f96 100644
> > --- a/tests/qemu-iotests/common.config
> > +++ b/tests/qemu-iotests/common.config
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/env bash
> > +# -*- emacs mode: sh -*-
> 
> I thought my version:
> # hey emacs, this file will be sourced by bash -*- mode: sh -*-
> was cuter, but that's not a requirement, and yours works  ;)
> 

I will not make any disputes on that! :)

Thanks for the reviews!
- Cleber.

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 

Re: [PATCH v2 3/4] qemu-iotests: 044: pass is actually a noop, so remove it

[Philippe Mathieu-Daudé]

On 10/9/19 9:47 PM, Cleber Rosa wrote:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/qemu-iotests/044 | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
> index 8b2afa2a11..aa2a00ceed 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -102,17 +102,14 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>       def setUp(self):
>           qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=512', test_img, '16G')
>           self.preallocate(test_img)
> -        pass
>   
>   
>       def tearDown(self):
>           os.remove(test_img)
> -        pass
>   
>       def test_grow_refcount_table(self):
>           qemu_io('-c', 'write 3800M 1M', test_img)
>           qemu_img_verbose('check' , test_img)
> -        pass
>   
>   if __name__ == '__main__':
>       iotests.main(supported_fmts=['qcow2'],
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Due to not being able to find a reason to have shebangs on files that
> are not executable.
> 
> While at it, add a mode hint to emacs, which would be clueless or
> plain wrong about these containing shell code.

vim still doesn't like the change.

Of course, we could also add another line for vim and for every other
editor in use, but actually, I think I'd prefer just dropping this
patch. It even makes each file a few bytes larger instead of saving
something. Shebang lines are a shorter and more portable format
indicator than the alternatives.

So I think in the end we have found a good reason to keep them. :-)

Kevin

Re: [PATCH v2 2/4] qemu-iotests: remove forceful execution success from library files

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Should not be necessary on files that are not executed standalone.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Looks fine for common.filter and common.rc, nobody ever checks their
return value.

common.config is included like this:

    if ! . "$source_iotests/common.config"
    then
        _init_error "failed to source common.config"
    fi

So as long as we keep this, don't we want to make sure that it returns
success?

Of course, we never really want to return an error from common.config,
so instead of keeping the final 'true' statement, we might consider
changing its inclusion to not check for errors. The case that
potentially changes is when common.config doesn't exist or isn't
readable, but this isn't supposed to ever happen anyway.

Kevin

Re: [PATCH v2 3/4] qemu-iotests: 044: pass is actually a noop, so remove it

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH v2 4/4] qemu-iotests: 044: remove inaccurate docstring class description

[Kevin Wolf]

Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Nir Soffer]

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

On Fri, Oct 11, 2019, 12:36 Kevin Wolf <kwolf@redhat.com> wrote:

> Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> > Due to not being able to find a reason to have shebangs on files that
> > are not executable.
> >
> > While at it, add a mode hint to emacs, which would be clueless or
> > plain wrong about these containing shell code.
>
> vim still doesn't like the change.
>
> Of course, we could also add another line for vim and for every other
> editor in use, but actually, I think I'd prefer just dropping this
> patch. It even makes each file a few bytes larger instead of saving
> something. Shebang lines are a shorter and more portable format
> indicator than the alternatives.
>
> So I think in the end we have found a good reason to keep them. :-)
>

What about .sh suffix? Should be most portable way.

>

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

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Cleber Rosa]

On Fri, Oct 11, 2019 at 02:27:25PM +0300, Nir Soffer wrote:
> On Fri, Oct 11, 2019, 12:36 Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> > > Due to not being able to find a reason to have shebangs on files that
> > > are not executable.
> > >
> > > While at it, add a mode hint to emacs, which would be clueless or
> > > plain wrong about these containing shell code.
> >
> > vim still doesn't like the change.
> >
> > Of course, we could also add another line for vim and for every other
> > editor in use, but actually, I think I'd prefer just dropping this
> > patch. It even makes each file a few bytes larger instead of saving
> > something. Shebang lines are a shorter and more portable format
> > indicator than the alternatives.
> >
> > So I think in the end we have found a good reason to keep them. :-)
> >
> 
> What about .sh suffix? Should be most portable way.
> 
> >

That's the approach I tend to follow for my sh code.  Explicit is
better than implicit if you ask me.

Kevin,

Do you have any strong feelings here?  I'd be fine with either this
or dropping the patch.

Thanks,
- Cleber.

Re: [PATCH v2 1/4] qemu-iotests: remove bash shebang from library files

[Kevin Wolf]

Am 11.10.2019 um 22:05 hat Cleber Rosa geschrieben:
> On Fri, Oct 11, 2019 at 02:27:25PM +0300, Nir Soffer wrote:
> > On Fri, Oct 11, 2019, 12:36 Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > > Am 09.10.2019 um 21:47 hat Cleber Rosa geschrieben:
> > > > Due to not being able to find a reason to have shebangs on files that
> > > > are not executable.
> > > >
> > > > While at it, add a mode hint to emacs, which would be clueless or
> > > > plain wrong about these containing shell code.
> > >
> > > vim still doesn't like the change.
> > >
> > > Of course, we could also add another line for vim and for every other
> > > editor in use, but actually, I think I'd prefer just dropping this
> > > patch. It even makes each file a few bytes larger instead of saving
> > > something. Shebang lines are a shorter and more portable format
> > > indicator than the alternatives.
> > >
> > > So I think in the end we have found a good reason to keep them. :-)
> > 
> > What about .sh suffix? Should be most portable way.
> 
> That's the approach I tend to follow for my sh code.  Explicit is
> better than implicit if you ask me.

I would certainly agree for new files.

> Kevin,
> 
> Do you have any strong feelings here?  I'd be fine with either this
> or dropping the patch.

No strong feelings. The result of renaming the files would be a bit
nicer than what we have today, but renaming always comes with a cost
when working with the version history later. Hard to tell if it's a net
gain or loss in the end.

Myself, I would probably pick the lazy way and stick with "if it ain't
broke, don't fix it", but I'm not objecting to a change either.

Kevin