Re: [PATCH v3 2/5] python/qemu: accel: Add list_accel() method

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

On Thu, Dec 12, 2019 at 07:58:28AM -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 <wainersm@redhat.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

The degree of changes is certainly subjective, but consider clearing
"Reviewed-by"s, according to:

https://wiki.qemu.org/Contribute/SubmitAPatch#Proper_use_of_Reviewed-by:_tags_can_aid_review

Alex, Phillipe,

You re-review on this patch is highly appreciated anyway.

> ---
>  python/qemu/accel.py | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index cbeac10dd1..ddcdbfd9ae 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([qemu_bin, '-accel', 'help'],
> +                                      universal_newlines=True)
> +    except:

This is a "generally frowned upon" naked except, but given that its
goal is to present the error to the user, and that it re-raises the
exception, it's much less frowned upon, so it LGTM.

> +        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)

The ideal use of the logging module log functions is to let them
format the output, passing those values as arguments, ie:

   LOG.debug("Failed to get the list of accelerators in %s", qemu_bin)

See https://docs.python.org/3.7/library/logging.html#logging.Logger.debug

And sorry for failing to catch this on v2.

- Cleber.

> +        raise
> +    # Skip the first line which is the header.
> +    return [acc.strip() for acc in out.splitlines()[1:]]
> +
>  def kvm_available(target_arch=None):
>      host_arch = os.uname()[4]
>      if target_arch and target_arch != host_arch:
> -- 
> 2.21.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]