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

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

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

Hi,

On 6/8/21 8:55 PM, Cleber Rosa Junior wrote:
>
>
> On Tue, Jun 8, 2021 at 5:42 PM Wainer dos Santos Moschetta 
> <wainersm@redhat.com <mailto:wainersm@redhat.com>> wrote:
>
>     Hi,
>
>     On 6/8/21 11: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
>     <mailto: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.
>     Cleber, please, tell me how is the future like! :)
>
>
> I grabbed a sports almanac.  That's all I can say. :)
>
> Now seriously, thanks for spotting the typo.
>
>     > +#
>     > +# Authors:
>     > +#  Cleber Rosa <crosa@redhat.com <mailto: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.
>
>     I understand we need a mean to easily cancel the test if given
>     feature
>     is not present. However, I'm unsure this generic list_feature() is
>     what
>     we need.
>
>     The `-accel help` returns a simple list of strings (besides the
>     header,
>     which is dismissed). Whereas `-machine help` returns what could be
>     parsed as a tuple of (name, description).
>
>     Another example is `-cpu help` which will print a similar list as
>     `-machine`, plus a section with CPUID flags.
>
>
> I made sure it worked with both "accel" and "machine", but you're 
> right about many other "-$feature help" that won't conform to the 
> mapping ("-chardev help" is probably the only other one that should 
> work).  What I thought about was to keep the same list_feature(), but 
> make its parsing of items flexible.  Then it could be reused for other 
> list_$feature() like methods.  At the same time, it could be an 
> opportunity to standardize a bit more of the "help" outputs.
>
> For instance, I think it would make sense for "cpu" to keep showing 
> the amount of information it shows, but:
>
> 1) The first item could be the name of the relevant "option" (the cpu 
> model) for that feature (cpu), instead of, say, "x86". Basically 
> reversing the order of first and second, or first and third fields.
> 2) Everything *after* an empty line would be extra information, not 
> necessarily an option available for that feature.
>
> But, this is most probably not going to be achieved in the short term.
>
> What I can do here, is to hide list_feature(), say call it 
> _list_feature() so that we don't duplicate code, and expose a 
> list_machine().

I prefer that implementation, with list_machine(), list_accel()...etc. 
Allow me another suggestion: maybe rename list_feature() to 
feature_help() (or something similar).

- Wainer

>
> Let me know what you think.
>
> Thanks,
> - Cleber.
>
>     If confess I still don't have a better idea, although I feel it will
>     require a OO design.
>
>     Thanks!
>
>     - Wainer
>
>     > +
>     > +    @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:]]
>

[-- Attachment #2: Type: text/html, Size: 12089 bytes --]