On Tue, Jun 8, 2021 at 8:55 PM Cleber Rosa Junior <cr...@redhat.com> wrote:
>
>
>
> On Tue, Jun 8, 2021 at 5:42 PM Wainer dos Santos Moschetta
> <waine...@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 <cr...@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 <cr...@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:]]
>>
