Re: [PATCH v2 2/4] python/qemu: accel: Add list_accel() method

[Cleber Rosa]

On Fri, Dec 06, 2019 at 04:34:31PM -0500, Wainer dos Santos Moschetta wrote:
> Since commit cbe6d6365a48 the command `qemu -accel help` returns
> the list of accelerators enabled in the QEMU binary. This adds
> the list_accel() method which return that same list.
> 
> Signed-off-by: Wainer dos Santos Moschetta <address@hidden>
> Reviewed-by: Alex Bennée <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  python/qemu/accel.py | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index cbeac10dd1..746b7e68f5 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -14,7 +14,11 @@ accelerators.
>  # the COPYING file in the top-level directory.
>  #
>  
> +import logging
>  import os
> +import subprocess
> +
> +LOG = logging.getLogger(__name__)
>  
>  # Mapping host architecture to any additional architectures it can
>  # support which often includes its 32 bit cousin.
> @@ -23,6 +27,25 @@ ADDITIONAL_ARCHES = {
>      "aarch64" : "armhf"
>  }
>  
> +def list_accel(qemu_bin):
> +    """
> +    List accelerators enabled in the QEMU binary.
> +
> +    @param qemu_bin (str): path to the QEMU binary.
> +    @raise Exception: if failed to run `qemu -accel help`
> +    @return a list of accelerator names.
> +    """
> +    if not qemu_bin:
> +        return []
> +    try:
> +        out = subprocess.check_output("%s -accel help" % qemu_bin, 
> shell=True)

There's no need to use a shell here.  This could become:

   out = subprocess.check_output([qemu_bin, '-accel' 'help'])

> +    except:
> +        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)
> +        raise
> +    lines = out.decode().splitlines()

And maybe discard the first line earlier with:

   lines = out.decode().splitlines()[1:]

Also, you could avoid the manual decode() with the `universal_newlines`
option to subprocess.check_output(), ie:

   accels = subprocess.check_output([qemu-bin, '-accel', 'help'],
                                    universal_newlines=True).splitlines()[1:]

> +    # Skip the first line which is the header.
> +    return [l.strip() for l in lines[1:] if l]
> +

I think that the `if l` check can actually hide undesirable behavior
(bugs) in the `qemu -accel ?` output.  I don't remember seeing
`-$(option) ?` returning empty strings but doesn't mean it couldn't
and shouldn't).

I do remember `-machine ?` returning random non-printable characters
that turned out to be a bug, though.

>  def kvm_available(target_arch=None):
>      host_arch = os.uname()[4]
>      if target_arch and target_arch != host_arch:
> -- 
> 2.21.0
> 

- Cleber.

signature.asc
Description: PGP signature