| Message ID | 20201008202713.1416823-4-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some crashes when using -object | expand |
On Thu, Oct 08, 2020 at 04:27:13PM -0400, Eduardo Habkost wrote: > Add a simple test case that will run QEMU directly (without QMP) > just to check for crashes when using `-object`. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Cc: Cleber Rosa <crosa@redhat.com> > Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com> > Cc: Wainer dos Santos Moschetta <wainersm@redhat.com> > Cc: qemu-devel@nongnu.org > --- > tests/acceptance/object_option.py | 49 +++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 tests/acceptance/object_option.py > > diff --git a/tests/acceptance/object_option.py b/tests/acceptance/object_option.py > new file mode 100644 > index 0000000000..2b8bd00db1 > --- /dev/null > +++ b/tests/acceptance/object_option.py > @@ -0,0 +1,49 @@ > +# Copyright (c) 2020 Red Hat, Inc. > +# > +# Author: > +# Eduardo Habkost <ehabkost@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 avocado_qemu > +import subprocess > +import shlex > + > +class ObjectOption(avocado_qemu.Test): > + """Check if ``-object`` option behaves as expected""" > + > + def run(self, cmd, *args, **kwargs): > + cmdstr = ' '.join(shlex.quote(c) for c in cmd) > + self.log.info("Command: %s", cmdstr) Maybe "Running command: %s" is clearer? > + return subprocess.run(cmd, encoding='utf-8', *args, **kwargs) I'd just use `universal_newlines=True` (equivalent to `text`, but that's Python 3.7+ only), so the current encoding will be respected. I understand "utf-8" is a safe choice, but IMO, if someone sets a different default, it should be respected, unless there's a clear reason not to do so. > + > + def devices(self): Maybe call it `get_devices()` or make it a property? > + out = self.run([self.qemu_bin, '-object', 'help'], > + check=True, stdout=subprocess.PIPE).stdout > + lines = out.split('\n') > + return [l.strip() for l in lines[1:] if l.strip()] > + In case the `CalledProcessError` is raised on subprocess.run(), the following test status will be ERROR, which is a condition you were not testing for, something unexpected. Given that you're using `check=True`, I'd decorate this test with: @avocado.fail_on(subprocess.CalledProcessError) > + def test_help(self): > + """Check if ``-object ...,help`` behaves as expected""" > + for device in self.devices(): > + self.run([self.qemu_bin, '-object', '%s,help' % (device)], > + check=True, > + stdout=subprocess.DEVNULL) > + > + def test_crash(self): > + """Check for crashes when using ``-object ...``""" Maybe change the wording here, given that this is really checking that QEMU doesn't crash, right? So something like "Checks that QEMU doesn't crash ..." seems clearer to me. > + for device in self.devices(): > + r = self.run([self.qemu_bin, '-object', > + '%s,id=obj0' % (device), > + '-monitor', 'stdio'], > + input='quit\n', > + stdout=subprocess.DEVNULL, > + stderr=subprocess.PIPE) I know adding command line options to QEMU (specially at this stage) is, at the very least, frowned upon, but I can't help to think that an option similar to Python's "-c" would be very helpful in testing scenarios. > + if r.returncode not in (0, 1): > + self.log.warn("QEMU stderr: %s", r.stderr) > + self.log.warn("QEMU exit code: %d", r.returncode) > + if r.returncode < 0: > + self.fail("QEMU crashed") > + else: > + self.fail("Unexpected exit code") > -- > 2.26.2 > This looks fine, but this test is clearly provocative (at least to me), given at it's a bit more barebones than the approached used in "empty_cpu_model.py". It seems to show the need for: 1) a custom test class (similar to avocado_qemu.Test) that can be more suitable to more barebones tests like this 2) a custom QEMU "varianter" implementation we've talked about in the past, with which the test code could be simplified. For instance, setting hypothetical configuration "qemu-varianter=objects" could produce: [ {"object": "authz-list"}, {"object": "authz-list-file"}, ... {"object": "tls-creds-x509"} ] I had not attempted this before, because Avocado was limited to applying variants globally to a job. Starting with version 81.0 (released a month or so ago), this is no longer a limitation. A simple PoC is here: https://gitlab.com/cleber.gnu/qemu/-/commit/30f26b662326502c2d82aabca225009ccdebe6aa Can be run with: ./tests/venv/bin/python job_object_option.py And the tests themselves would look like this: https://paste.centos.org/view/4c5f413d Let me know if any of this makes sense. Thanks, - Cleber.
diff --git a/tests/acceptance/object_option.py b/tests/acceptance/object_option.py new file mode 100644 index 0000000000..2b8bd00db1 --- /dev/null +++ b/tests/acceptance/object_option.py @@ -0,0 +1,49 @@ +# Copyright (c) 2020 Red Hat, Inc. +# +# Author: +# Eduardo Habkost <ehabkost@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 avocado_qemu +import subprocess +import shlex + +class ObjectOption(avocado_qemu.Test): + """Check if ``-object`` option behaves as expected""" + + def run(self, cmd, *args, **kwargs): + cmdstr = ' '.join(shlex.quote(c) for c in cmd) + self.log.info("Command: %s", cmdstr) + return subprocess.run(cmd, encoding='utf-8', *args, **kwargs) + + def devices(self): + out = self.run([self.qemu_bin, '-object', 'help'], + check=True, stdout=subprocess.PIPE).stdout + lines = out.split('\n') + return [l.strip() for l in lines[1:] if l.strip()] + + def test_help(self): + """Check if ``-object ...,help`` behaves as expected""" + for device in self.devices(): + self.run([self.qemu_bin, '-object', '%s,help' % (device)], + check=True, + stdout=subprocess.DEVNULL) + + def test_crash(self): + """Check for crashes when using ``-object ...``""" + for device in self.devices(): + r = self.run([self.qemu_bin, '-object', + '%s,id=obj0' % (device), + '-monitor', 'stdio'], + input='quit\n', + stdout=subprocess.DEVNULL, + stderr=subprocess.PIPE) + if r.returncode not in (0, 1): + self.log.warn("QEMU stderr: %s", r.stderr) + self.log.warn("QEMU exit code: %d", r.returncode) + if r.returncode < 0: + self.fail("QEMU crashed") + else: + self.fail("Unexpected exit code")
Add a simple test case that will run QEMU directly (without QMP) just to check for crashes when using `-object`. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Cleber Rosa <crosa@redhat.com> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com> Cc: Wainer dos Santos Moschetta <wainersm@redhat.com> Cc: qemu-devel@nongnu.org --- tests/acceptance/object_option.py | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tests/acceptance/object_option.py