[PATCH v3 0/5] python/qemu: New accel module and improvements

[PATCH v3 0/5] 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.

Patch 05 just remove unneeded imports in __init__.py

Changes v2 -> v3:
- Refactor subprocess.check_output() call (patch 02) [crosa]
  Not using shell=True
  Pass universal_newlines=True so don't need to decode() the output
  Do not check if returned accelerator's name is empty string
- New patch 05 [crosa]
  On patch 01 Cleber suggested to remove unneeded imports in
  python/qemu/__init__.py

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_v3

CI:
- Travis (FAIL): https://travis-ci.org/wainersm/qemu/builds/623800273
  Failure is not related with this change

Wainer dos Santos Moschetta (5):
  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: Remove unneeded imports in __init__

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

-- 
2.21.0

[PATCH v3 1/5] 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 v3 2/5] 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: Cleber Rosa <crosa@redhat.com>
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..ddcdbfd9ae 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([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:]]
+
 def kvm_available(target_arch=None):
     host_arch = os.uname()[4]
     if target_arch and target_arch != host_arch:
-- 
2.21.0

[PATCH v3 3/5] 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: Cleber Rosa <crosa@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Cleber Rosa <crosa@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 ddcdbfd9ae..833f9431ce 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 [acc.strip() for acc in out.splitlines()[1:]]
 
-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 v3 4/5] 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: Cleber Rosa <crosa@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Cleber Rosa <crosa@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 833f9431ce..2823bb0cc3 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

[PATCH v3 5/5] python/qemu: Remove unneeded imports in __init__

[Wainer dos Santos Moschetta]

__init_.py import some sub-modules unnecessarily. So let's
clean it up.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Suggested-by: Cleber Rosa <crosa@redhat.com>
---
 python/qemu/__init__.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index eff17a306e..4ca06c34a4 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -9,9 +9,3 @@
 # This work is licensed under the terms of the GNU GPL, version 2.  See
 # the COPYING file in the top-level directory.
 #
-# Based on qmp.py.
-#
-
-from . import qmp
-from . import machine
-from . import accel
-- 
2.21.0

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

[Cleber Rosa]

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

On Thu, Dec 12, 2019 at 07:58:27AM -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>

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 v3 2/5] python/qemu: accel: Add list_accel() method

[Cleber Rosa]

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

On Thu, Dec 12, 2019 at 07:58:28AM -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: Cleber Rosa <crosa@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

The degree of changes is certainly subjective, but consider clearing
"Reviewed-by"s, according to:

https://wiki.qemu.org/Contribute/SubmitAPatch#Proper_use_of_Reviewed-by:_tags_can_aid_review

Alex, Phillipe,

You re-review on this patch is highly appreciated anyway.

> ---
>  python/qemu/accel.py | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/python/qemu/accel.py b/python/qemu/accel.py
> index cbeac10dd1..ddcdbfd9ae 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([qemu_bin, '-accel', 'help'],
> +                                      universal_newlines=True)
> +    except:

This is a "generally frowned upon" naked except, but given that its
goal is to present the error to the user, and that it re-raises the
exception, it's much less frowned upon, so it LGTM.

> +        LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin)

The ideal use of the logging module log functions is to let them
format the output, passing those values as arguments, ie:

   LOG.debug("Failed to get the list of accelerators in %s", qemu_bin)

See https://docs.python.org/3.7/library/logging.html#logging.Logger.debug

And sorry for failing to catch this on v2.

- Cleber.

> +        raise
> +    # Skip the first line which is the header.
> +    return [acc.strip() for acc in out.splitlines()[1:]]
> +
>  def kvm_available(target_arch=None):
>      host_arch = os.uname()[4]
>      if target_arch and target_arch != host_arch:
> -- 
> 2.21.0
> 

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

Re: [PATCH v3 5/5] python/qemu: Remove unneeded imports in __init__

[Cleber Rosa]

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

On Thu, Dec 12, 2019 at 07:58:31AM -0500, Wainer dos Santos Moschetta wrote:
> __init_.py import some sub-modules unnecessarily. So let's
> clean it up.
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Suggested-by: Cleber Rosa <crosa@redhat.com>
> ---
>  python/qemu/__init__.py | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index eff17a306e..4ca06c34a4 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -9,9 +9,3 @@
>  # This work is licensed under the terms of the GNU GPL, version 2.  See
>  # the COPYING file in the top-level directory.
>  #
> -# Based on qmp.py.
> -#
> -
> -from . import qmp
> -from . import machine
> -from . import accel
> -- 
> 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 --]