[PATCH v2 0/4] python/qemu: New accel module and improvements

[PATCH v2 0/4] python/qemu: New accel module and improvements

[Wainer dos Santos Moschetta]

On commit abf0bf998dcb John Snow moved some code out of __init__.py
to machine.py. kvm_available() remained in though. So on patch 01
I continue his work by creating a home for that method (the new
'accel' module). Honestly I was unsure about whether move the code
to any existing module or make a new, but since I am adding more
methods related with accelerators then I thought they would deserve a module.

The patches 02-04 introduce new helpers and make improvements. Later
I intend to use those methods on the acceptance tests such as
to automatically set the accelerator in QEMUMachine VM via Avocado
tags, and skip the test if the accelerator is not available.

Changes v1 -> v2:
- Removed 'Based on qmp.py' from python/qemu/accel.py
  (patch 01) [alex.bennee]
- logging added only when used on python/qemu/accel.py
  (patch 02) [alex.bennee]

Git:
- Tree: https://github.com/wainersm/qemu
- Branch: python_accel_v2

CI:
- Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/621748861


Wainer dos Santos Moschetta (4):
  python/qemu: Move kvm_available() to its own module
  python/qemu: accel: Add list_accel() method
  python/qemu: accel: Strengthen kvm_available() checks
  python/qemu: accel: Add tcg_available() method

 python/qemu/__init__.py | 20 +----------
 python/qemu/accel.py    | 77 +++++++++++++++++++++++++++++++++++++++++
 tests/vm/basevm.py      |  2 +-
 3 files changed, 79 insertions(+), 20 deletions(-)
 create mode 100644 python/qemu/accel.py

-- 
2.21.0

[PATCH v2 1/4] python/qemu: Move kvm_available() to its own module

[Wainer dos Santos Moschetta]

This creates the 'accel' Python module to be the home for
utilities that deal with accelerators. Also moved kvm_available()
from __init__.py to this new module.

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/__init__.py | 20 +-------------------
 python/qemu/accel.py    | 31 +++++++++++++++++++++++++++++++
 tests/vm/basevm.py      |  2 +-
 3 files changed, 33 insertions(+), 20 deletions(-)
 create mode 100644 python/qemu/accel.py

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 6c919a3d56..eff17a306e 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -12,24 +12,6 @@
 # Based on qmp.py.
 #
 
-import logging
-import os
-
 from . import qmp
 from . import machine
-
-LOG = logging.getLogger(__name__)
-
-# Mapping host architecture to any additional architectures it can
-# support which often includes its 32 bit cousin.
-ADDITIONAL_ARCHES = {
-    "x86_64" : "i386",
-    "aarch64" : "armhf"
-}
-
-def kvm_available(target_arch=None):
-    host_arch = os.uname()[4]
-    if target_arch and target_arch != host_arch:
-        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
-            return False
-    return os.access("/dev/kvm", os.R_OK | os.W_OK)
+from . import accel
diff --git a/python/qemu/accel.py b/python/qemu/accel.py
new file mode 100644
index 0000000000..cbeac10dd1
--- /dev/null
+++ b/python/qemu/accel.py
@@ -0,0 +1,31 @@
+"""
+QEMU accel module:
+
+This module provides utilities for discover and check the availability of
+accelerators.
+"""
+# Copyright (C) 2015-2016 Red Hat Inc.
+# Copyright (C) 2012 IBM Corp.
+#
+# Authors:
+#  Fam Zheng <famz@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 os
+
+# Mapping host architecture to any additional architectures it can
+# support which often includes its 32 bit cousin.
+ADDITIONAL_ARCHES = {
+    "x86_64" : "i386",
+    "aarch64" : "armhf"
+}
+
+def kvm_available(target_arch=None):
+    host_arch = os.uname()[4]
+    if target_arch and target_arch != host_arch:
+        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
+            return False
+    return os.access("/dev/kvm", os.R_OK | os.W_OK)
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 91a9226026..3e2b69c96c 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -21,7 +21,7 @@ import logging
 import time
 import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import kvm_available
+from qemu.accel import kvm_available
 from qemu.machine import QEMUMachine
 import subprocess
 import hashlib
-- 
2.21.0

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

[Wainer dos Santos Moschetta]

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)
+    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:
-- 
2.21.0

[PATCH v2 3/4] python/qemu: accel: Strengthen kvm_available() checks

[Wainer dos Santos Moschetta]

Currently kvm_available() checks for the presence of kvm module
and, if target and host arches don't mismatch. This patch adds
an 3rd checking: if QEMU binary was compiled with kvm
support.

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 | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 746b7e68f5..9fecffb44b 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -46,9 +46,24 @@ def list_accel(qemu_bin):
     # 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:
-        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
-            return False
-    return os.access("/dev/kvm", os.R_OK | os.W_OK)
+def kvm_available(target_arch=None, qemu_bin=None):
+    """
+    Check if KVM is available using the following heuristic:
+      - Kernel module is present in the host;
+      - Target and host arches don't mismatch;
+      - KVM is enabled in the QEMU binary.
+
+    @param target_arch (str): target architecture
+    @param qemu_bin (str): path to the QEMU binary
+    @return True if kvm is available, otherwise False.
+    """
+    if not os.access("/dev/kvm", os.R_OK | os.W_OK):
+        return False
+    if target_arch:
+        host_arch = os.uname()[4]
+        if target_arch != host_arch:
+            if target_arch != ADDITIONAL_ARCHES.get(host_arch):
+                return False
+    if qemu_bin and "kvm" not in list_accel(qemu_bin):
+        return False
+    return True
-- 
2.21.0

[PATCH v2 4/4] python/qemu: accel: Add tcg_available() method

[Wainer dos Santos Moschetta]

This adds a method to check if the tcg accelerator is enabled
in the QEMU binary.

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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/python/qemu/accel.py b/python/qemu/accel.py
index 9fecffb44b..f4ff31825d 100644
--- a/python/qemu/accel.py
+++ b/python/qemu/accel.py
@@ -67,3 +67,11 @@ def kvm_available(target_arch=None, qemu_bin=None):
     if qemu_bin and "kvm" not in list_accel(qemu_bin):
         return False
     return True
+
+def tcg_available(qemu_bin):
+    """
+    Check if TCG is available.
+
+    @param qemu_bin (str): path to the QEMU binary
+    """
+    return 'tcg' in list_accel(qemu_bin)
-- 
2.21.0

Re: [PATCH v2 1/4] python/qemu: Move kvm_available() to its own module

[Cleber Rosa]

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

On Fri, Dec 06, 2019 at 04:34:30PM -0500, Wainer dos Santos Moschetta wrote:
> This creates the 'accel' Python module to be the home for
> utilities that deal with accelerators. Also moved kvm_available()
> from __init__.py to this new module.
> 
> 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/__init__.py | 20 +-------------------
>  python/qemu/accel.py    | 31 +++++++++++++++++++++++++++++++
>  tests/vm/basevm.py      |  2 +-
>  3 files changed, 33 insertions(+), 20 deletions(-)
>  create mode 100644 python/qemu/accel.py
> 
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index 6c919a3d56..eff17a306e 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -12,24 +12,6 @@
>  # Based on qmp.py.
>  #
>  
> -import logging
> -import os
> -
>  from . import qmp
>  from . import machine

These lines don't achieve anything besides forcing an earlier
evaluation of those modules, which AFAICT is totally unnecessary.
If there was code in QEMU such as:

  import qemu

  def foo():
     ...
     raise qemu.qmp.QMPError

Then there would be a reason for such imports.  I couldn't find
any though.  Anyway, I now this is not from your patch but...

> -
> -LOG = logging.getLogger(__name__)
> -
> -# Mapping host architecture to any additional architectures it can
> -# support which often includes its 32 bit cousin.
> -ADDITIONAL_ARCHES = {
> -    "x86_64" : "i386",
> -    "aarch64" : "armhf"
> -}
> -
> -def kvm_available(target_arch=None):
> -    host_arch = os.uname()[4]
> -    if target_arch and target_arch != host_arch:
> -        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> -            return False
> -    return os.access("/dev/kvm", os.R_OK | os.W_OK)
> +from . import accel

... according to that reasoning, this should be ommited.  IMO we could
use another patch in this series removing those imports.

> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> new file mode 100644
> index 0000000000..cbeac10dd1
> --- /dev/null
> +++ b/python/qemu/accel.py
> @@ -0,0 +1,31 @@
> +"""
> +QEMU accel module:
> +
> +This module provides utilities for discover and check the availability of
> +accelerators.
> +"""
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  Fam Zheng <famz@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 os
> +
> +# Mapping host architecture to any additional architectures it can
> +# support which often includes its 32 bit cousin.
> +ADDITIONAL_ARCHES = {
> +    "x86_64" : "i386",
> +    "aarch64" : "armhf"
> +}
> +
> +def kvm_available(target_arch=None):
> +    host_arch = os.uname()[4]
> +    if target_arch and target_arch != host_arch:
> +        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> +            return False
> +    return os.access("/dev/kvm", os.R_OK | os.W_OK)
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 91a9226026..3e2b69c96c 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -21,7 +21,7 @@ import logging
>  import time
>  import datetime
>  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> -from qemu import kvm_available
> +from qemu.accel import kvm_available
>  from qemu.machine import QEMUMachine
>  import subprocess
>  import hashlib
> -- 
> 2.21.0
> 

Other than that, this LGTM.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Cleber Rosa]

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] python/qemu: accel: Strengthen kvm_available() checks

[Cleber Rosa]

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

On Fri, Dec 06, 2019 at 04:34:32PM -0500, Wainer dos Santos Moschetta wrote:
> Currently kvm_available() checks for the presence of kvm module
> and, if target and host arches don't mismatch. This patch adds
> an 3rd checking: if QEMU binary was compiled with kvm
> support.
> 
> 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 | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index 746b7e68f5..9fecffb44b 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -46,9 +46,24 @@ def list_accel(qemu_bin):
>      # 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:
> -        if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> -            return False
> -    return os.access("/dev/kvm", os.R_OK | os.W_OK)
> +def kvm_available(target_arch=None, qemu_bin=None):
> +    """
> +    Check if KVM is available using the following heuristic:
> +      - Kernel module is present in the host;
> +      - Target and host arches don't mismatch;
> +      - KVM is enabled in the QEMU binary.
> +
> +    @param target_arch (str): target architecture
> +    @param qemu_bin (str): path to the QEMU binary
> +    @return True if kvm is available, otherwise False.
> +    """
> +    if not os.access("/dev/kvm", os.R_OK | os.W_OK):
> +        return False
> +    if target_arch:
> +        host_arch = os.uname()[4]
> +        if target_arch != host_arch:
> +            if target_arch != ADDITIONAL_ARCHES.get(host_arch):
> +                return False
> +    if qemu_bin and "kvm" not in list_accel(qemu_bin):
> +        return False
> +    return True
> -- 
> 2.21.0
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/4] python/qemu: accel: Add tcg_available() method

[Cleber Rosa]

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

On Fri, Dec 06, 2019 at 04:34:33PM -0500, Wainer dos Santos Moschetta wrote:
> This adds a method to check if the tcg accelerator is enabled
> in the QEMU binary.
> 
> 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 | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index 9fecffb44b..f4ff31825d 100644
> --- a/python/qemu/accel.py
> +++ b/python/qemu/accel.py
> @@ -67,3 +67,11 @@ def kvm_available(target_arch=None, qemu_bin=None):
>      if qemu_bin and "kvm" not in list_accel(qemu_bin):
>          return False
>      return True
> +
> +def tcg_available(qemu_bin):
> +    """
> +    Check if TCG is available.
> +
> +    @param qemu_bin (str): path to the QEMU binary
> +    """
> +    return 'tcg' in list_accel(qemu_bin)
> -- 
> 2.21.0
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wainer dos Santos Moschetta]

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.

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

[Cleber Rosa]

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

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.
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]