Re: [PATCH 2/4] Python QEMU utils: introduce a generic feature list

[John Snow <jsnow@redhat.com>]

On 6/8/21 10:09 AM, Cleber Rosa wrote:
> Which can be used to check for any "feature" that is available as a
> QEMU command line option, and that will return its list of available
> options.
> 
> This is a generalization of the list_accel() utility function, which
> is itself re-implemented in terms of the more generic feature.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   python/qemu/utils/__init__.py |  2 ++
>   python/qemu/utils/accel.py    | 15 ++----------
>   python/qemu/utils/feature.py  | 44 +++++++++++++++++++++++++++++++++++
>   3 files changed, 48 insertions(+), 13 deletions(-)
>   create mode 100644 python/qemu/utils/feature.py
> 
> diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
> index 7f1a5138c4..1d0789eaa2 100644
> --- a/python/qemu/utils/__init__.py
> +++ b/python/qemu/utils/__init__.py
> @@ -20,12 +20,14 @@
>   
>   # pylint: disable=import-error
>   from .accel import kvm_available, list_accel, tcg_available
> +from .feature import list_feature
>   
>   
>   __all__ = (
>       'get_info_usernet_hostfwd_port',
>       'kvm_available',
>       'list_accel',
> +    'list_feature',
>       'tcg_available',
>   )
>   
> diff --git a/python/qemu/utils/accel.py b/python/qemu/utils/accel.py
> index 297933df2a..b5bb80c6d3 100644
> --- a/python/qemu/utils/accel.py
> +++ b/python/qemu/utils/accel.py
> @@ -14,13 +14,11 @@
>   # the COPYING file in the top-level directory.
>   #
>   
> -import logging
>   import os
> -import subprocess
>   from typing import List, Optional
>   
> +from qemu.utils.feature import list_feature
>   
> -LOG = logging.getLogger(__name__)
>   
>   # Mapping host architecture to any additional architectures it can
>   # support which often includes its 32 bit cousin.
> @@ -39,16 +37,7 @@ def list_accel(qemu_bin: str) -> List[str]:
>       @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:
> -        LOG.debug("Failed to get the list of accelerators in %s", qemu_bin)
> -        raise
> -    # Skip the first line which is the header.
> -    return [acc.strip() for acc in out.splitlines()[1:]]
> +    return list_feature(qemu_bin, 'accel')
>   
>   
>   def kvm_available(target_arch: Optional[str] = None,
> diff --git a/python/qemu/utils/feature.py b/python/qemu/utils/feature.py
> new file mode 100644
> index 0000000000..b4a5f929ab
> --- /dev/null
> +++ b/python/qemu/utils/feature.py
> @@ -0,0 +1,44 @@
> +"""
> +QEMU feature module:
> +
> +This module provides a utility for discovering the availability of
> +generic features.
> +"""
> +# Copyright (C) 2022 Red Hat Inc.
> +#
> +# Authors:
> +#  Cleber Rosa <crosa@redhat.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +import logging
> +import subprocess
> +from typing import List
> +
> +
> +LOG = logging.getLogger(__name__)
> +
> +
> +def list_feature(qemu_bin: str, feature: str) -> List[str]:
> +    """
> +    List available options the QEMU binary for a given feature type.
> +
> +    By calling a "qemu $feature -help" and parsing its output.
> +
> +    @param qemu_bin (str): path to the QEMU binary.
> +    @param feature (str): feature name, matching the command line option.
> +    @raise Exception: if failed to run `qemu -feature help`
> +    @return a list of available options for the given feature.
> +    """
> +    if not qemu_bin:
> +        return []
> +    try:
> +        out = subprocess.check_output([qemu_bin, '-%s' % feature, 'help'],
> +                                      universal_newlines=True)
> +    except:
> +        LOG.debug("Failed to get the list of %s(s) in %s", feature, qemu_bin)
> +        raise
> +    # Skip the first line which is the header.
> +    return [item.split(' ', 1)[0] for item in out.splitlines()[1:]]
> 

It's messy stuff, but all of machine.py is pretty messy stuff right now. 
No real qualms with more messy stuff going into qemu.utils for the time 
being.

Eventually, we will want to come up with a more universal way to 
interrogate features present in QEMU binaries. Using introspection data 
or QMP queries would be my preferred (and ideally SOLE) way to detect 
QEMU features.

But that's something to worry about later, I suppose.

As long as it passes the CI and doesn't break any tests, I'll toss you 
my ACK here and trust your judgment:

Acked-by: John Snow <jsnow@redhat.com>

--js