[Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Wainer dos Santos Moschetta]

QEMU used to exits with a not accurate error message when
an initrd >= 2GB was passed. That was fixed on patch:

	commit f3839fda5771596152b75dd1e1a6d050e6e6e380
	Author: Li Zhijian <lizhijian@cn.fujitsu.com>
	Date:   Thu Sep 13 18:07:13 2018 +0800

    	change get_image_size return type to int64_t

This change adds a regression test for that fix. It starts
QEMU with a 2GB dummy initrd, and check it evaluates the file
size correctly and prints accurate message.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 tests/acceptance/linux_initrd.py | 47 ++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 tests/acceptance/linux_initrd.py

diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
new file mode 100644
index 0000000000..7d9e5862cd
--- /dev/null
+++ b/tests/acceptance/linux_initrd.py
@@ -0,0 +1,47 @@
+# Linux initrd acceptance test.
+#
+# Copyright (c) 2018 Red Hat, Inc.
+#
+# Author:
+#  Wainer dos Santos Moschetta <wainersm@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+import tempfile
+
+from avocado_qemu import Test
+from avocado.utils.process import run
+
+
+class LinuxInitrd(Test):
+    """
+    Checks QEMU evaluates correctly the initrd file passed as -initrd option.
+
+    :avocado: enable
+    :avocado: tags=x86_64
+    """
+
+    timeout = 60
+
+    def test_with_2GB_file_should_exit_error_msg(self):
+        """
+        Pretends to boot QEMU with an initrd file with size of 2GB
+        and expect it exits with error message.
+        Regression test for bug fixed on commit f3839fda5771596152.
+        """
+        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
+                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
+        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
+        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
+        with tempfile.NamedTemporaryFile() as initrd:
+            initrd.seek(2048*(1024**2) -1)
+            initrd.write(b'\0')
+            initrd.flush()
+            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
+                                                initrd.name)
+            res = run(cmd, ignore_status=True)
+            self.assertNotEqual(res.exit_status, 0)
+            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
+            self.assertRegex(res.stderr_text, expected_msg)
-- 
2.17.2

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Philippe Mathieu-Daudé]

On 18/10/2018 18:20, Wainer dos Santos Moschetta wrote:
> QEMU used to exits with a not accurate error message when
> an initrd >= 2GB was passed. That was fixed on patch:
> 
> 	commit f3839fda5771596152b75dd1e1a6d050e6e6e380
> 	Author: Li Zhijian <lizhijian@cn.fujitsu.com>
> 	Date:   Thu Sep 13 18:07:13 2018 +0800
> 
>     	change get_image_size return type to int64_t
> 
> This change adds a regression test for that fix. It starts
> QEMU with a 2GB dummy initrd, and check it evaluates the file
> size correctly and prints accurate message.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/linux_initrd.py | 47 ++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 tests/acceptance/linux_initrd.py
> 
> diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
> new file mode 100644
> index 0000000000..7d9e5862cd
> --- /dev/null
> +++ b/tests/acceptance/linux_initrd.py
> @@ -0,0 +1,47 @@
> +# Linux initrd acceptance test.
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import tempfile
> +
> +from avocado_qemu import Test
> +from avocado.utils.process import run
> +
> +
> +class LinuxInitrd(Test):
> +    """
> +    Checks QEMU evaluates correctly the initrd file passed as -initrd option.
> +
> +    :avocado: enable
> +    :avocado: tags=x86_64

I'll let Cleber check if he wants we use 'tags=arch:x86_64' here.

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


> +    """
> +
> +    timeout = 60
> +
> +    def test_with_2GB_file_should_exit_error_msg(self):
> +        """
> +        Pretends to boot QEMU with an initrd file with size of 2GB
> +        and expect it exits with error message.
> +        Regression test for bug fixed on commit f3839fda5771596152.
> +        """
> +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
> +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
> +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +
> +        with tempfile.NamedTemporaryFile() as initrd:
> +            initrd.seek(2048*(1024**2) -1)
> +            initrd.write(b'\0')
> +            initrd.flush()
> +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
> +                                                initrd.name)
> +            res = run(cmd, ignore_status=True)
> +            self.assertNotEqual(res.exit_status, 0)
> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
> +            self.assertRegex(res.stderr_text, expected_msg)
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Caio Carrara]

Hi Wainer,

On 18-10-2018 13:20, Wainer dos Santos Moschetta wrote:
> [...]
> +    def test_with_2GB_file_should_exit_error_msg(self):
> +        """
> +        Pretends to boot QEMU with an initrd file with size of 2GB
> +        and expect it exits with error message.
> +        Regression test for bug fixed on commit f3839fda5771596152.
> +        """
> +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
> +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
> +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +
> +        with tempfile.NamedTemporaryFile() as initrd:
> +            initrd.seek(2048*(1024**2) -1)
> +            initrd.write(b'\0')
> +            initrd.flush()
> +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
> +                                                initrd.name)
> +            res = run(cmd, ignore_status=True)
> +            self.assertNotEqual(res.exit_status, 0)
> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
> +            self.assertRegex(res.stderr_text, expected_msg)
> 

At least on my run this test is the first one to fail (with error) due
Python version (on Py2 there's no assertRegex method). AFIK we're moving
towards only Py3 support on the acceptance tests, right? The current
behavior (error) is the expected?

Since the comment above is just for clarification and not a blocking issue:

Reviewed-by: Caio Carrara <ccarrara@redhat.com>
Tested-by: Caio Carrara <ccarrara@redhat.com>

-- 
Caio Carrara
Software Engineer, Virt Team
Red Hat
ccarrara@redhat.com

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Cleber Rosa]

On 10/18/18 12:44 PM, Philippe Mathieu-Daudé wrote:
> On 18/10/2018 18:20, Wainer dos Santos Moschetta wrote:
>> QEMU used to exits with a not accurate error message when
>> an initrd >= 2GB was passed. That was fixed on patch:
>>
>> 	commit f3839fda5771596152b75dd1e1a6d050e6e6e380
>> 	Author: Li Zhijian <lizhijian@cn.fujitsu.com>
>> 	Date:   Thu Sep 13 18:07:13 2018 +0800
>>
>>     	change get_image_size return type to int64_t
>>
>> This change adds a regression test for that fix. It starts
>> QEMU with a 2GB dummy initrd, and check it evaluates the file
>> size correctly and prints accurate message.
>>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> ---
>>  tests/acceptance/linux_initrd.py | 47 ++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 tests/acceptance/linux_initrd.py
>>
>> diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
>> new file mode 100644
>> index 0000000000..7d9e5862cd
>> --- /dev/null
>> +++ b/tests/acceptance/linux_initrd.py
>> @@ -0,0 +1,47 @@
>> +# Linux initrd acceptance test.
>> +#
>> +# Copyright (c) 2018 Red Hat, Inc.
>> +#
>> +# Author:
>> +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later.  See the COPYING file in the top-level directory.
>> +
>> +import tempfile
>> +
>> +from avocado_qemu import Test
>> +from avocado.utils.process import run
>> +
>> +
>> +class LinuxInitrd(Test):
>> +    """
>> +    Checks QEMU evaluates correctly the initrd file passed as -initrd option.
>> +
>> +    :avocado: enable
>> +    :avocado: tags=x86_64
> 
> I'll let Cleber check if he wants we use 'tags=arch:x86_64' here.
> 

Yes, I have no objections.  Like I said elsewhere, most good standards
are born like this.

- Cleber.

> Regardless:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 
>> +    """
>> +
>> +    timeout = 60
>> +
>> +    def test_with_2GB_file_should_exit_error_msg(self):
>> +        """
>> +        Pretends to boot QEMU with an initrd file with size of 2GB
>> +        and expect it exits with error message.
>> +        Regression test for bug fixed on commit f3839fda5771596152.
>> +        """
>> +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
>> +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
>> +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
>> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>> +
>> +        with tempfile.NamedTemporaryFile() as initrd:
>> +            initrd.seek(2048*(1024**2) -1)
>> +            initrd.write(b'\0')
>> +            initrd.flush()
>> +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
>> +                                                initrd.name)
>> +            res = run(cmd, ignore_status=True)
>> +            self.assertNotEqual(res.exit_status, 0)
>> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
>> +            self.assertRegex(res.stderr_text, expected_msg)
>>

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Cleber Rosa]

On 10/18/18 3:11 PM, Caio Carrara wrote:
> Hi Wainer,
> 
> On 18-10-2018 13:20, Wainer dos Santos Moschetta wrote:
>> [...]
>> +    def test_with_2GB_file_should_exit_error_msg(self):
>> +        """
>> +        Pretends to boot QEMU with an initrd file with size of 2GB
>> +        and expect it exits with error message.
>> +        Regression test for bug fixed on commit f3839fda5771596152.
>> +        """
>> +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
>> +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
>> +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
>> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>> +
>> +        with tempfile.NamedTemporaryFile() as initrd:
>> +            initrd.seek(2048*(1024**2) -1)
>> +            initrd.write(b'\0')
>> +            initrd.flush()
>> +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
>> +                                                initrd.name)
>> +            res = run(cmd, ignore_status=True)
>> +            self.assertNotEqual(res.exit_status, 0)
>> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
>> +            self.assertRegex(res.stderr_text, expected_msg)
>>
> 
> At least on my run this test is the first one to fail (with error) due
> Python version (on Py2 there's no assertRegex method). AFIK we're moving
> towards only Py3 support on the acceptance tests, right? The current
> behavior (error) is the expected?
> 

We have verbalized and agreed on that indeed.  Wainer, can you please
add a simple line in the acceptance tests documentation mentioning that?

- Cleber.

> Since the comment above is just for clarification and not a blocking issue:
> 
> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
> Tested-by: Caio Carrara <ccarrara@redhat.com>
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Cleber Rosa]

On 10/18/18 12:20 PM, Wainer dos Santos Moschetta wrote:
> QEMU used to exits with a not accurate error message when
> an initrd >= 2GB was passed. That was fixed on patch:
> 
> 	commit f3839fda5771596152b75dd1e1a6d050e6e6e380
> 	Author: Li Zhijian <lizhijian@cn.fujitsu.com>
> 	Date:   Thu Sep 13 18:07:13 2018 +0800
> 
>     	change get_image_size return type to int64_t
> 
> This change adds a regression test for that fix. It starts
> QEMU with a 2GB dummy initrd, and check it evaluates the file
> size correctly and prints accurate message.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  tests/acceptance/linux_initrd.py | 47 ++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 tests/acceptance/linux_initrd.py
> 
> diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
> new file mode 100644
> index 0000000000..7d9e5862cd
> --- /dev/null
> +++ b/tests/acceptance/linux_initrd.py
> @@ -0,0 +1,47 @@
> +# Linux initrd acceptance test.
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import tempfile
> +
> +from avocado_qemu import Test
> +from avocado.utils.process import run
> +
> +
> +class LinuxInitrd(Test):
> +    """
> +    Checks QEMU evaluates correctly the initrd file passed as -initrd option.
> +
> +    :avocado: enable
> +    :avocado: tags=x86_64
> +    """
> +
> +    timeout = 60
> +
> +    def test_with_2GB_file_should_exit_error_msg(self):
> +        """
> +        Pretends to boot QEMU with an initrd file with size of 2GB
> +        and expect it exits with error message.
> +        Regression test for bug fixed on commit f3839fda5771596152.
> +        """
> +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
> +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
> +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> +
> +        with tempfile.NamedTemporaryFile() as initrd:
> +            initrd.seek(2048*(1024**2) -1)

It's a style issue, but I'd go with spaces between operators:

        initrd.seek(2048 * (1024 ** 2) - 1)

One other possibility that may improve code readability is:

        from avocado.utils.data_structures import DataSize
        initrd.seek(DataSize('2G').b - 1)

Or finally, just set a "max_size" variable to the 2GB literal value.

> +            initrd.write(b'\0')
> +            initrd.flush()
> +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
> +                                                initrd.name)
> +            res = run(cmd, ignore_status=True)
> +            self.assertNotEqual(res.exit_status, 0)
> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'

I'd be a bit more assertive here and do something like:

      expected_msg = r'.*initrd is too large.*max: \d+, need %d\)' %
max_size

And if "134053887" (which appears in "max"), is a QEMU constant, that
can be added there as well.

- Cleber.

> +            self.assertRegex(res.stderr_text, expected_msg)
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Eduardo Habkost]

On Thu, Oct 18, 2018 at 05:45:56PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/18/18 12:20 PM, Wainer dos Santos Moschetta wrote:
> > QEMU used to exits with a not accurate error message when
> > an initrd >= 2GB was passed. That was fixed on patch:
> > 
> > 	commit f3839fda5771596152b75dd1e1a6d050e6e6e380
> > 	Author: Li Zhijian <lizhijian@cn.fujitsu.com>
> > 	Date:   Thu Sep 13 18:07:13 2018 +0800
> > 
> >     	change get_image_size return type to int64_t
> > 
> > This change adds a regression test for that fix. It starts
> > QEMU with a 2GB dummy initrd, and check it evaluates the file
> > size correctly and prints accurate message.
> > 
> > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > ---
> >  tests/acceptance/linux_initrd.py | 47 ++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >  create mode 100644 tests/acceptance/linux_initrd.py
> > 
> > diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
> > new file mode 100644
> > index 0000000000..7d9e5862cd
> > --- /dev/null
> > +++ b/tests/acceptance/linux_initrd.py
> > @@ -0,0 +1,47 @@
> > +# Linux initrd acceptance test.
> > +#
> > +# Copyright (c) 2018 Red Hat, Inc.
> > +#
> > +# Author:
> > +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > +# later.  See the COPYING file in the top-level directory.
> > +
> > +import tempfile
> > +
> > +from avocado_qemu import Test
> > +from avocado.utils.process import run
> > +
> > +
> > +class LinuxInitrd(Test):
> > +    """
> > +    Checks QEMU evaluates correctly the initrd file passed as -initrd option.
> > +
> > +    :avocado: enable
> > +    :avocado: tags=x86_64
> > +    """
> > +
> > +    timeout = 60
> > +
> > +    def test_with_2GB_file_should_exit_error_msg(self):
> > +        """
> > +        Pretends to boot QEMU with an initrd file with size of 2GB
> > +        and expect it exits with error message.
> > +        Regression test for bug fixed on commit f3839fda5771596152.
> > +        """
> > +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
> > +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
> > +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
> > +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
> > +
> > +        with tempfile.NamedTemporaryFile() as initrd:
> > +            initrd.seek(2048*(1024**2) -1)
> 
> It's a style issue, but I'd go with spaces between operators:
> 
>         initrd.seek(2048 * (1024 ** 2) - 1)
> 
> One other possibility that may improve code readability is:
> 
>         from avocado.utils.data_structures import DataSize
>         initrd.seek(DataSize('2G').b - 1)
> 

I'd agree if "2G" weren't ambiguous (I would expect it to mean
2,000,000,000, not 1,073,741,824).

I can live with the ambiguity of 2GB in the commit message, class
name, and docstring.  But having to look for the
avocado.utils.data_structures.DataSize documentation to find out
what's the file size makes the code less readable to me.


> Or finally, just set a "max_size" variable to the 2GB literal value.
> 
> > +            initrd.write(b'\0')
> > +            initrd.flush()
> > +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
> > +                                                initrd.name)
> > +            res = run(cmd, ignore_status=True)
> > +            self.assertNotEqual(res.exit_status, 0)

I would prefer to ensure exit code is 1.  We don't want the test
to pass if QEMU crashed with a signal, for example.


> > +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
> 
> I'd be a bit more assertive here and do something like:
> 
>       expected_msg = r'.*initrd is too large.*max: \d+, need %d\)' %
> max_size
> 
> And if "134053887" (which appears in "max"), is a QEMU constant, that
> can be added there as well.

I was going to suggest the opposite: just checking if the error
message contains "initrd is too large".  If we ensure the exit
status is 1, the exact format of the error message isn't very
important.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Cleber Rosa]

On 10/18/18 6:09 PM, Eduardo Habkost wrote:
> On Thu, Oct 18, 2018 at 05:45:56PM -0400, Cleber Rosa wrote:
>>
>>
>> On 10/18/18 12:20 PM, Wainer dos Santos Moschetta wrote:
>>> QEMU used to exits with a not accurate error message when
>>> an initrd >= 2GB was passed. That was fixed on patch:
>>>
>>> 	commit f3839fda5771596152b75dd1e1a6d050e6e6e380
>>> 	Author: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> 	Date:   Thu Sep 13 18:07:13 2018 +0800
>>>
>>>     	change get_image_size return type to int64_t
>>>
>>> This change adds a regression test for that fix. It starts
>>> QEMU with a 2GB dummy initrd, and check it evaluates the file
>>> size correctly and prints accurate message.
>>>
>>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>> ---
>>>  tests/acceptance/linux_initrd.py | 47 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 47 insertions(+)
>>>  create mode 100644 tests/acceptance/linux_initrd.py
>>>
>>> diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
>>> new file mode 100644
>>> index 0000000000..7d9e5862cd
>>> --- /dev/null
>>> +++ b/tests/acceptance/linux_initrd.py
>>> @@ -0,0 +1,47 @@
>>> +# Linux initrd acceptance test.
>>> +#
>>> +# Copyright (c) 2018 Red Hat, Inc.
>>> +#
>>> +# Author:
>>> +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
>>> +#
>>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>>> +# later.  See the COPYING file in the top-level directory.
>>> +
>>> +import tempfile
>>> +
>>> +from avocado_qemu import Test
>>> +from avocado.utils.process import run
>>> +
>>> +
>>> +class LinuxInitrd(Test):
>>> +    """
>>> +    Checks QEMU evaluates correctly the initrd file passed as -initrd option.
>>> +
>>> +    :avocado: enable
>>> +    :avocado: tags=x86_64
>>> +    """
>>> +
>>> +    timeout = 60
>>> +
>>> +    def test_with_2GB_file_should_exit_error_msg(self):
>>> +        """
>>> +        Pretends to boot QEMU with an initrd file with size of 2GB
>>> +        and expect it exits with error message.
>>> +        Regression test for bug fixed on commit f3839fda5771596152.
>>> +        """
>>> +        kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
>>> +                      'Everything/x86_64/os/images/pxeboot/vmlinuz')
>>> +        kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
>>> +        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
>>> +
>>> +        with tempfile.NamedTemporaryFile() as initrd:
>>> +            initrd.seek(2048*(1024**2) -1)
>>
>> It's a style issue, but I'd go with spaces between operators:
>>
>>         initrd.seek(2048 * (1024 ** 2) - 1)
>>
>> One other possibility that may improve code readability is:
>>
>>         from avocado.utils.data_structures import DataSize
>>         initrd.seek(DataSize('2G').b - 1)
>>
> 
> I'd agree if "2G" weren't ambiguous (I would expect it to mean
> 2,000,000,000, not 1,073,741,824).
> 
> I can live with the ambiguity of 2GB in the commit message, class
> name, and docstring.  But having to look for the
> avocado.utils.data_structures.DataSize documentation to find out
> what's the file size makes the code less readable to me.
> 
> 

Fair enough.  The "G" here isn't clear about gibibyte/gigabyte.

>> Or finally, just set a "max_size" variable to the 2GB literal value.
>>
>>> +            initrd.write(b'\0')
>>> +            initrd.flush()
>>> +            cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
>>> +                                                initrd.name)
>>> +            res = run(cmd, ignore_status=True)
>>> +            self.assertNotEqual(res.exit_status, 0)
> 
> I would prefer to ensure exit code is 1.  We don't want the test
> to pass if QEMU crashed with a signal, for example.
> 
> 

Agreed.

>>> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
>>
>> I'd be a bit more assertive here and do something like:
>>
>>       expected_msg = r'.*initrd is too large.*max: \d+, need %d\)' %
>> max_size
>>
>> And if "134053887" (which appears in "max"), is a QEMU constant, that
>> can be added there as well.
> 
> I was going to suggest the opposite: just checking if the error
> message contains "initrd is too large".  If we ensure the exit
> status is 1, the exact format of the error message isn't very
> important.
> 

It's certainly a matter of style here, and both are fine to me, but I'd
rather have someone touching that part of the code to also have to touch
the test if the message changes.

Note that I can't predict if this will eventually catch a regression
that the simpler message you suggest wouldn't, but, I, personally,
prefer to be safe than sorry.

- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: add Linux initrd checking test

[Eduardo Habkost]

On Thu, Oct 18, 2018 at 07:52:58PM -0400, Cleber Rosa wrote:
[...]
> >>> +            expected_msg = r'.*initrd is too large.*max: \d+, need \d+.*'
> >>
> >> I'd be a bit more assertive here and do something like:
> >>
> >>       expected_msg = r'.*initrd is too large.*max: \d+, need %d\)' %
> >> max_size
> >>
> >> And if "134053887" (which appears in "max"), is a QEMU constant, that
> >> can be added there as well.
> > 
> > I was going to suggest the opposite: just checking if the error
> > message contains "initrd is too large".  If we ensure the exit
> > status is 1, the exact format of the error message isn't very
> > important.
> > 
> 
> It's certainly a matter of style here, and both are fine to me, but I'd
> rather have someone touching that part of the code to also have to touch
> the test if the message changes.
> 
> Note that I can't predict if this will eventually catch a regression
> that the simpler message you suggest wouldn't, but, I, personally,
> prefer to be safe than sorry.

I see your point, and I would probably prefer your approach if we
already had QEMU developers used to writing and running
acceptance tests once in a while.

Anyway, both approaches would be good enough to me.

-- 
Eduardo