[0/3] Fix some crashes when using -object

Fix two crashes when using `-object` with no extra arguments, and
add a acceptance test case to detect similar crashes in the
future.

Eduardo Habkost (3):
  authz-list-file: Fix crash when filename is not set
  can-host-socketcan: Fix crash when 'if' option is not set
  tests/acceptance: Test case for detecting -object crashes

 authz/listfile.c                  |  5 ++++
 net/can/can_socketcan.c           |  5 ++++
 tests/acceptance/object_option.py | 49 +++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)
 create mode 100644 tests/acceptance/object_option.py

-- 
2.26.2

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.

On 10/8/20 10:27 PM, Eduardo Habkost wrote:
> Fix the following crash:
> 
>    $ qemu-system-x86_64 -object authz-list-file,id=obj0
>    qemu-system-x86_64: -object authz-list-file,id=obj0: GLib: g_file_get_contents: assertion 'filename != NULL' failed
>    Segmentation fault (core dumped)
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>   authz/listfile.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/authz/listfile.c b/authz/listfile.c
> index cd6163aa40..aaf930453d 100644
> --- a/authz/listfile.c
> +++ b/authz/listfile.c
> @@ -122,6 +122,11 @@ qauthz_list_file_complete(UserCreatable *uc, Error **errp)
>       QAuthZListFile *fauthz = QAUTHZ_LIST_FILE(uc);
>       gchar *dir = NULL, *file = NULL;
>   
> +    if (!fauthz->filename) {
> +        error_setg(errp, "filename not provided");
> +        return;
> +    }
> +
>       fauthz->list = qauthz_list_file_load(fauthz, errp);
>   
>       if (!fauthz->refresh) {
> 

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