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

[Willian Rampazzo]

On Tue, Jun 8, 2021 at 8:55 PM Cleber Rosa Junior <crosa@redhat.com> wrote:
>
>
>
> On Tue, Jun 8, 2021 at 5:42 PM Wainer dos Santos Moschetta 
> <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>
>> > ---
>> >   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>
>> > +#
>> > +# 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 have reviewed the code and it looks good to me, but +1 for
`list_machine()` and other `list_<specific>` functions. We can handle
different cases on its own function and make it easier to use.

>
> 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:]]
>>