diff mbox series

[v2,2/4] python/qemu: accel: Add list_accel() method

Message ID 20191206213433.11305-3-wainersm@redhat.com (mailing list archive)
State New, archived
Headers show
Series python/qemu: New accel module and improvements | expand

Commit Message

Wainer dos Santos Moschetta Dec. 6, 2019, 9:34 p.m. UTC
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: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 python/qemu/accel.py | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Cleber Rosa Dec. 10, 2019, 12:52 a.m. UTC | #1
On Fri, Dec 06, 2019 at 04:34:31PM -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: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  python/qemu/accel.py | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index cbeac10dd1..746b7e68f5 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("%s -accel help" % qemu_bin, shell=True)

There's no need to use a shell here.  This could become:

   out = subprocess.check_output([qemu_bin, '-accel' 'help'])

> +    except:
> +        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)
> +        raise
> +    lines = out.decode().splitlines()

And maybe discard the first line earlier with:

   lines = out.decode().splitlines()[1:]

Also, you could avoid the manual decode() with the `universal_newlines`
option to subprocess.check_output(), ie:

   accels = subprocess.check_output([qemu-bin, '-accel', 'help'],
                                    universal_newlines=True).splitlines()[1:]

> +    # Skip the first line which is the header.
> +    return [l.strip() for l in lines[1:] if l]
> +

I think that the `if l` check can actually hide undesirable behavior
(bugs) in the `qemu -accel ?` output.  I don't remember seeing
`-$(option) ?` returning empty strings but doesn't mean it couldn't
and shouldn't).

I do remember `-machine ?` returning random non-printable characters
that turned out to be a bug, though.

>  def kvm_available(target_arch=None):
>      host_arch = os.uname()[4]
>      if target_arch and target_arch != host_arch:
> -- 
> 2.21.0
> 

- Cleber.
Wainer dos Santos Moschetta Dec. 11, 2019, 4:58 p.m. UTC | #2
On 12/9/19 10:52 PM, Cleber Rosa wrote:
> On Fri, Dec 06, 2019 at 04:34:31PM -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: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   python/qemu/accel.py | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
>> index cbeac10dd1..746b7e68f5 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("%s -accel help" % qemu_bin, shell=True)
> There's no need to use a shell here.  This could become:
>
>     out = subprocess.check_output([qemu_bin, '-accel' 'help'])

Ack

>
>> +    except:
>> +        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)
>> +        raise
>> +    lines = out.decode().splitlines()
> And maybe discard the first line earlier with:
>
>     lines = out.decode().splitlines()[1:]
>
> Also, you could avoid the manual decode() with the `universal_newlines`
> option to subprocess.check_output(), ie:
>
>     accels = subprocess.check_output([qemu-bin, '-accel', 'help'],
>                                      universal_newlines=True).splitlines()[1:]

Nice. v3 will have universal_newlines=True.

>
>> +    # Skip the first line which is the header.
>> +    return [l.strip() for l in lines[1:] if l]
>> +
> I think that the `if l` check can actually hide undesirable behavior
> (bugs) in the `qemu -accel ?` output.  I don't remember seeing
> `-$(option) ?` returning empty strings but doesn't mean it couldn't
> and shouldn't).
>
> I do remember `-machine ?` returning random non-printable characters
> that turned out to be a bug, though.

Double-checking: are you suggesting to remove the 'if not empty' check 
so that bugs on output could emerge?

Thanks!

- Wainer

>
>>   def kvm_available(target_arch=None):
>>       host_arch = os.uname()[4]
>>       if target_arch and target_arch != host_arch:
>> -- 
>> 2.21.0
>>
> - Cleber.
Cleber Rosa Dec. 11, 2019, 5:46 p.m. UTC | #3
On Wed, Dec 11, 2019 at 02:58:35PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 12/9/19 10:52 PM, Cleber Rosa wrote:
> > On Fri, Dec 06, 2019 at 04:34:31PM -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: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > ---
> > >   python/qemu/accel.py | 23 +++++++++++++++++++++++
> > >   1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> > > index cbeac10dd1..746b7e68f5 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("%s -accel help" % qemu_bin, shell=True)
> > There's no need to use a shell here.  This could become:
> > 
> >     out = subprocess.check_output([qemu_bin, '-accel' 'help'])
> 
> Ack
> 
> > 
> > > +    except:
> > > +        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)
> > > +        raise
> > > +    lines = out.decode().splitlines()
> > And maybe discard the first line earlier with:
> > 
> >     lines = out.decode().splitlines()[1:]
> > 
> > Also, you could avoid the manual decode() with the `universal_newlines`
> > option to subprocess.check_output(), ie:
> > 
> >     accels = subprocess.check_output([qemu-bin, '-accel', 'help'],
> >                                      universal_newlines=True).splitlines()[1:]
> 
> Nice. v3 will have universal_newlines=True.
> 
> > 
> > > +    # Skip the first line which is the header.
> > > +    return [l.strip() for l in lines[1:] if l]
> > > +
> > I think that the `if l` check can actually hide undesirable behavior
> > (bugs) in the `qemu -accel ?` output.  I don't remember seeing
> > `-$(option) ?` returning empty strings but doesn't mean it couldn't
> > and shouldn't).
> > 
> > I do remember `-machine ?` returning random non-printable characters
> > that turned out to be a bug, though.
> 
> Double-checking: are you suggesting to remove the 'if not empty' check so
> that bugs on output could emerge?
>

Yes, that's my suggestion.  I don't think we need to process QEMU's
output beyond what it's expected to be returned.

Cheers,
- Cleber.

> Thanks!
> 
> - Wainer
> 
> > 
> > >   def kvm_available(target_arch=None):
> > >       host_arch = os.uname()[4]
> > >       if target_arch and target_arch != host_arch:
> > > -- 
> > > 2.21.0
> > > 
> > - Cleber.
> 
>
diff mbox series

Patch

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index cbeac10dd1..746b7e68f5 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("%s -accel help" % qemu_bin, shell=True)
+    except:
+        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)
+        raise
+    lines = out.decode().splitlines()
+    # Skip the first line which is the header.
+    return [l.strip() for l in lines[1:] if l]
+
 def kvm_available(target_arch=None):
     host_arch = os.uname()[4]
     if target_arch and target_arch != host_arch: