[PATCH 0/4] iotests: trivial cleanups

[PATCH 0/4] iotests: trivial cleanups

[Cleber Rosa]

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

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     | 1 -
 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, 26 deletions(-)

-- 
2.21.0

[PATCH 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.

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     | 1 -
 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, 13 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 9bd1a5a6fc..6956d38d4c 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -1,5 +1,3 @@
-#!/usr/bin/env bash
-#
 # 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 445a1c23e0..043c62c10c 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -1,5 +1,3 @@
-#!/usr/bin/env bash
-#
 # 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..5a9991b7ef 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -1,4 +1,3 @@
-#!/usr/bin/env bash
 # -*- shell-script-mode -*-
 #
 # Helpers for NBD server related config
diff --git a/tests/qemu-iotests/common.pattern b/tests/qemu-iotests/common.pattern
index 4f5e5bcea0..e8d97dd2bb 100644
--- a/tests/qemu-iotests/common.pattern
+++ b/tests/qemu-iotests/common.pattern
@@ -1,5 +1,3 @@
-#!/usr/bin/env bash
-#
 # Copyright (C) 2009 Red Hat, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 8d2021a7eb..5bdfde890d 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -1,5 +1,3 @@
-#!/usr/bin/env bash
-#
 # 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..19bddacf11 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -1,5 +1,3 @@
-#!/usr/bin/env bash
-#
 # 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..61f8ef6037 100644
--- a/tests/qemu-iotests/common.tls
+++ b/tests/qemu-iotests/common.tls
@@ -1,5 +1,3 @@
-#!/usr/bin/env bash
-#
 # Helpers for TLS related config
 #
 # Copyright (C) 2018 Red Hat, Inc.
-- 
2.21.0

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

[Cleber Rosa]

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

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 6956d38d4c..0a24d960ff 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -34,6 +34,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 043c62c10c..55db15865a 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -221,6 +221,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 19bddacf11..d7e6456086 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -600,6 +600,3 @@ _require_drivers()
         fi
     done
 }
-
-# make sure this script returns success
-true
-- 
2.21.0

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

[Cleber Rosa]

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 05ea1f49c5..eb42df0fe1 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -105,17 +105,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 4/4] qemu-iotests: 044: remove inaccurate docstring class description

[Cleber Rosa]

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 eb42df0fe1..0ca4bcfc6d 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -34,7 +34,6 @@ if sys.version_info.major == 2:
 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 1/4] qemu-iotests: remove bash shebang from library files

[Eric Blake]

On 9/27/19 9:17 AM, Cleber Rosa wrote:
> Due to not being able to find a reason to have shebangs on files that
> are not executable.
> 
> 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     | 1 -
>   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, 13 deletions(-)
> 

Loss of the shebang changes the mode in which emacs opens the files 
(from Shell-script[bash] to Conf[space] in my case).  I agree that a #! 
comment is not appropriate for a file that is not executable as a 
standalone file, but it becomes harder to edit the file correctly unless 
we replace it with some other way of letting editors realize that the 
contents of each file is still meant to be consumed by bash.

Something like this would work:

# hey emacs, this file will be sourced by bash: -*- sh -*-

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

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

[Eric Blake]

On 9/27/19 9:17 AM, Cleber Rosa wrote:
> Should not be necessary on files that are not executed standalone.
> 
> 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 6956d38d4c..0a24d960ff 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -34,6 +34,3 @@ _optstr_add()
>           echo "$2"
>       fi
>   }
> -
> -# make sure this script returns success
> -true

The exit status of the source command in the caller depends on the last 
command executed here.

However, you also have the point that if you delete this line, the last 
command executed is a function definition which is successful (for all 3 
files touched).  So there is no behavior change in dropping this line.

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 3/4] qemu-iotests: 044: pass is actually a noop, so remove it

[Eric Blake]

On 9/27/19 9:17 AM, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/qemu-iotests/044 | 3 ---
>   1 file changed, 3 deletions(-)

It's useful when there is nothing else, but here we definitely have 
something else.

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

> 
> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
> index 05ea1f49c5..eb42df0fe1 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -105,17 +105,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'],
> 

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

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

[Eric Blake]

On 9/27/19 9:17 AM, Cleber Rosa wrote:
> 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 eb42df0fe1..0ca4bcfc6d 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -34,7 +34,6 @@ if sys.version_info.major == 2:
>   test_img = os.path.join(iotests.test_dir, 'test.img')
>   
>   class TestRefcountTableGrowth(iotests.QMPTestCase):
> -    '''Abstract base class for image mirroring test cases'''
>   

Should we replace it with something useful?  But this isn't meant to be 
reused outside this test, so it doesn't hurt to have no documentation.

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

>       def preallocate(self, name):
>           fd = open(name, "r+b")
> 

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

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

[John Snow]

On 9/27/19 10:17 AM, Cleber Rosa wrote:
> Signed-off-by: Cleber Rosa <crosa@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On 9/27/19 10:17 AM, Cleber Rosa wrote:
> 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 eb42df0fe1..0ca4bcfc6d 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -34,7 +34,6 @@ if sys.version_info.major == 2:
>  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")
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 0/4] iotests: trivial cleanups

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 923 bytes --]

On 27.09.19 16:17, Cleber Rosa wrote:
> The most trivial set of cleanups to iotests common libraries and the
> 044 test.
> 
> 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     | 1 -
>  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, 26 deletions(-)

Looks OK to me, but I’d like to know what you think about Eric’s concern
on patch 1.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Cleber Rosa]

On Fri, Sep 27, 2019 at 11:37:52AM -0500, Eric Blake wrote:
> On 9/27/19 9:17 AM, Cleber Rosa wrote:
> > Due to not being able to find a reason to have shebangs on files that
> > are not executable.
> > 
> > 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     | 1 -
> >   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, 13 deletions(-)
> > 
> 
> Loss of the shebang changes the mode in which emacs opens the files (from
> Shell-script[bash] to Conf[space] in my case).  I agree that a #! comment is
> not appropriate for a file that is not executable as a standalone file, but
> it becomes harder to edit the file correctly unless we replace it with some
> other way of letting editors realize that the contents of each file is still
> meant to be consumed by bash.
> 
> Something like this would work:
> 
> # hey emacs, this file will be sourced by bash: -*- sh -*-
>

Yes, good point.  Will send that on a v2.

- Cleber.

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

Re: [PATCH 0/4] iotests: trivial cleanups

[Cleber Rosa]

On Mon, Oct 07, 2019 at 03:09:25PM +0200, Max Reitz wrote:
> On 27.09.19 16:17, Cleber Rosa wrote:
> > The most trivial set of cleanups to iotests common libraries and the
> > 044 test.
> > 
> > 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     | 1 -
> >  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, 26 deletions(-)
> 
> Looks OK to me, but I’d like to know what you think about Eric’s concern
> on patch 1.
> 
> Max
> 

Yep, I agree with Eric's point.  Will send a v2 shortly.

Thanks,
- Cleber.

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

[Cleber Rosa]

On Wed, Oct 09, 2019 at 12:26:27PM -0400, Cleber Rosa wrote:
> On Fri, Sep 27, 2019 at 11:37:52AM -0500, Eric Blake wrote:
> > On 9/27/19 9:17 AM, Cleber Rosa wrote:
> > > Due to not being able to find a reason to have shebangs on files that
> > > are not executable.
> > > 
> > > 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     | 1 -
> > >   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, 13 deletions(-)
> > > 
> > 
> > Loss of the shebang changes the mode in which emacs opens the files (from
> > Shell-script[bash] to Conf[space] in my case).  I agree that a #! comment is
> > not appropriate for a file that is not executable as a standalone file, but
> > it becomes harder to edit the file correctly unless we replace it with some
> > other way of letting editors realize that the contents of each file is still
> > meant to be consumed by bash.
> > 
> > Something like this would work:
> > 
> > # hey emacs, this file will be sourced by bash: -*- sh -*-
> >
> 
> Yes, good point.  Will send that on a v2.
>

BTW, in addition to that, we may add to .editorconfig something like:

[tests/qemu-iotests/common.*]
indent_style = space
indent_size = 4
file_type_emacs = sh

Although I was expecting editorconfig to provide a mode hint for other
editors, which doesn't seem to be the case.

Cheers,
- Cleber.

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