[PATCH v2 1/1] Jobs based on custom runners: add CentOS Stream 8

[Cleber Rosa]

This introduces three different parts of a job designed to run
on a custom runner managed by Red Hat.  The goals include:

  a) propose a model for other organizations that want to onboard
 their own runners, with their specific platforms, build
 configuration and tests.

  b) bring awareness to the differences between upstream QEMU and the
 version available under CentOS Stream, which is "A preview of
 upcoming Red Hat Enterprise Linux minor and major releases".

  c) because of b), it should be easier to identify and reduce the gap
 between Red Hat's downstream and upstream QEMU.

The components of this custom job are:

  I) OS build environment setup code:

 - additions to the existing "build-environment.yml" playbook
   that can be used to set up CentOS/EL 8 systems.

 - a CentOS Stream 8 specific "build-environment.yml" playbook
   that adds to the generic one.

 II) QEMU build configuration: a script that will produce binaries with
 features as similar as possible to the ones built and packaged on
 CentOS stream 8.

III) Scripts that define the minimum amount of testing that the
 binaries built with the given configuration (point II) under the
 given OS build environment (point I) should be subjected to.

 IV) Job definition: GitLab CI jobs that will dispatch the build/test
 jobs (see points #II and #III) to the machine specifically
 configured according to #I.

Signed-off-by: Cleber Rosa 
---
 .gitlab-ci.d/custom-runners.yml   |  29 +++
 docs/devel/ci-jobs.rst.inc|   7 +
 .../org.centos/stream/8/build-environment.yml |  51 +
 .../ci/org.centos/stream/8/x86_64/configure   | 208 ++
 .../org.centos/stream/8/x86_64/test-avocado   |  70 ++
 scripts/ci/org.centos/stream/README   |  17 ++
 scripts/ci/setup/build-environment.yml|  38 
 7 files changed, 420 insertions(+)
 create mode 100644 scripts/ci/org.centos/stream/8/build-environment.yml
 create mode 100755 scripts/ci/org.centos/stream/8/x86_64/configure
 create mode 100755 scripts/ci/org.centos/stream/8/x86_64/test-avocado
 create mode 100644 scripts/ci/org.centos/stream/README

diff --git a/.gitlab-ci.d/custom-runners.yml b/.gitlab-ci.d/custom-runners.yml
index a89a20da48..1f56297dfa 100644
--- a/.gitlab-ci.d/custom-runners.yml
+++ b/.gitlab-ci.d/custom-runners.yml
@@ -248,3 +248,32 @@ ubuntu-20.04-aarch64-notcg:
  - ../configure --disable-libssh --disable-tcg
  - make --output-sync -j`nproc`
  - make --output-sync -j`nproc` check V=1
+
+centos-stream-8-x86_64:
+ allow_failure: true
+ needs: []
+ stage: build
+ tags:
+ - centos_stream_8
+ - x86_64
+ rules:
+ - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ 
/^staging/'
+ - if: "$CENTOS_STREAM_8_x86_64_RUNNER_AVAILABLE"
+ artifacts:
+   name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
+   when: on_failure
+   expire_in: 7 days
+   paths:
+ - build/tests/results/latest/results.xml
+ - build/tests/results/latest/test-results
+   reports:
+ junit: build/tests/results/latest/results.xml
+ before_script:
+ - JOBS=$(expr $(nproc) + 1)
+ script:
+ - mkdir build
+ - cd build
+ - ../scripts/ci/org.centos/stream/8/x86_64/configure
+ - make -j"$JOBS"
+ - make NINJA=":" check
+ - ../scripts/ci/org.centos/stream/8/x86_64/test-avocado
diff --git a/docs/devel/ci-jobs.rst.inc b/docs/devel/ci-jobs.rst.inc
index 277975e4ad..db3f571d5f 100644
--- a/docs/devel/ci-jobs.rst.inc
+++ b/docs/devel/ci-jobs.rst.inc
@@ -49,3 +49,10 @@ S390X_RUNNER_AVAILABLE
 If you've got access to an IBM Z host that can be used as a gitlab-CI
 runner, you can set this variable to enable the tests that require this
 kind of host. The runner should be tagged with "s390x".
+
+CENTOS_STREAM_8_x86_64_RUNNER_AVAILABLE
+~~~
+If you've got access to a CentOS Stream 8 x86_64 host that can be
+used as a gitlab-CI runner, you can set this variable to enable the
+tests that require this kind of host. The runner should be tagged with
+both "centos_stream_8" and "x86_64".
diff --git a/scripts/ci/org.centos/stream/8/build-environment.yml 
b/scripts/ci/org.centos/stream/8/build-environment.yml
new file mode 100644
index 00..42b0471634
--- /dev/null
+++ b/scripts/ci/org.centos/stream/8/build-environment.yml
@@ -0,0 +1,51 @@
+---
+- name: Installation of extra packages to build QEMU
+  hosts: all
+  tasks:
+- name: Extra check for CentOS Stream 8
+  lineinfile:
+path: /etc/redhat-release
+line: CentOS Stream release 8
+state: present
+  check_mode: yes
+  register: centos_stream_8
+
+- name: Enable PowerTools repo on CentOS Stream 8
+  ini_file:
+path: /etc/yum.repos.d/CentOS-Stream-PowerTools.repo
+section: powertools
+option: enabled
+  

[PATCH v2 0/1] Jobs based on custom runners: add CentOS Stream 8

[Cleber Rosa]

This adds a new custom runner, showing an example of how other
entities can add their own custom jobs to the GitLab CI pipeline.

The runner (the machine and job) is to be managed by Red Hat, and
adds, at the very least, bare metal x86_64 KVM testing capabilities to
the QEMU pipeline.  This brings extra coverage for some unittests, and
the ability to run the Avocado tests that depend on KVM.

The runner is already completely set up and registered to the
https://gitlab.com/qemu-project/qemu project instance.  Jobs will be
triggered according to the same rules for the jobs s390x and aarch64
jobs running on QEMU project's custom runners, that is, pushes to the
staging branch of the "qemu-project" project, or by setting a specific
variable.

Still, the job is set with mode "allow failures", so it should not
disrupt the existing pipeline.  Once its reliability is proved (rules
and service levels are to be determined), it can be "upgraded" to
a "gating" condition.

Even though the formal method of tracking machine/job maintainers have
not been formalized, it should be known that the contacts/admins for
this machine and job are:

 - Willian Rampazzo
   
   willianr on #qemu

 - Cleber Rosa
   
   clebergnu on #qemu

One example of a job introduced here, running on the host reserved for
this purpose can be seen at:

 - https://gitlab.com/cleber.gnu/qemu/-/jobs/1773761640

Changes from v1[1]:

 * Replaced "--disable-fdt" for "--enable-fdt", given that according
   to "TARGET_NEED_FDT=y" in "configs/targets/x86_64-softmmu.mak" it
   is required for x86_64-softmmu.

 * Added libfdt-devel to list of package requirements (see previous
   point for reasoning).

 * Removed patch 1 that contained a duplicate bug fix.

 * Removed patches 2 and 3 that implemented a "feature probe" and
   "feature requirement" that would cancel tests if features were not
   present.  That will be treated in a different patch series.

 * Removed --disable-jemalloc and --disabletcmalloc according to
   3b4da1329.

 * Introduced "test-avocado" script with a list of vetted tests

 * Do not install meson from CentOS Stream 8 PowerTools repo, instead
   meson from git submodule due to minimum version requirements.

 * Sync with commit f68d21ab8eac56c4097a3d63a8c86689bb507911 (HEAD of
   c8s-stream-rhel branch) from CentOS repo at
   https://git.centos.org/rpms/qemu-kvm/.

 * Further separated distribution version and architecture specific
   files into separate sub directories.

 * Added a gitlab CI rule and variable to allow other repos/users who
   have a CentOS Stream 8 x86_64 runner to trigger the job.

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg02066.html

Cleber Rosa (1):
  Jobs based on custom runners: add CentOS Stream 8

 .gitlab-ci.d/custom-runners.yml   |  29 +++
 docs/devel/ci-jobs.rst.inc|   7 +
 .../org.centos/stream/8/build-environment.yml |  51 +
 .../ci/org.centos/stream/8/x86_64/configure   | 208 ++
 .../org.centos/stream/8/x86_64/test-avocado   |  70 ++
 scripts/ci/org.centos/stream/README   |  17 ++
 scripts/ci/setup/build-environment.yml|  38 
 7 files changed, 420 insertions(+)
 create mode 100644 scripts/ci/org.centos/stream/8/build-environment.yml
 create mode 100755 scripts/ci/org.centos/stream/8/x86_64/configure
 create mode 100755 scripts/ci/org.centos/stream/8/x86_64/test-avocado
 create mode 100644 scripts/ci/org.centos/stream/README

-- 
2.33.1

Re: [PATCH 4/9] tests/acceptance: Extract image_expand() helper

[Cleber Rosa]

Philippe Mathieu-Daudé writes:

> To be able to expand an image to a non-power-of-2 value,
> extract image_expand() from image_pow2ceil_expand().
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py 
> b/tests/acceptance/boot_linux_console.py
> index 20d57c1a8c6..61069f0064f 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -35,15 +35,19 @@
>  def pow2ceil(x):
>  return 1 if x == 0 else 2**(x - 1).bit_length()
>  
> +"""
> +Expand file size
> +"""
> +def image_expand(path, size):
> +if size != os.path.getsize(path):
> +with open(path, 'ab+') as fd:
> +fd.truncate(size)
> +

This would be a good time to make the comment section, into a proper
docstring, that is:

def image_expand(path, size):
   """Expand file size"""
if size != os.path.getsize(path):
with open(path, 'ab+') as fd:
fd.truncate(size)

- Cleber.

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Cleber Rosa]

Philippe Mathieu-Daudé writes:

> Avocado allows us to select set of tests using tags.
> When wanting to run all tests using a NetBSD guest OS,
> it is convenient to have them tagged, add the 'os:netbsd'
> tag.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 1 +
>  tests/acceptance/ppc_prep_40p.py   | 2 ++
>  2 files changed, 3 insertions(+)

Reviewed-by: Cleber Rosa 

Phil,

I can ammend the commit message and queue this one if you think it's a
good idea.

Cheers,
- Cleber.

Re: [PATCH 4/4] Jobs based on custom runners: add CentOS Stream 8

[Cleber Rosa Junior]

On Tue, Jun 8, 2021 at 10:10 AM Cleber Rosa  wrote:
>
> This introduces three different parts of a job designed to run
> on a custom runner managed by Red Hat.  The goals include:
>
>  a) serve as a model for other organizations that want to onboard
> their own runners, with their specific platforms, build
> configuration and tests.
>
>  b) bring awareness to the differences between upstream QEMU and the
> version available under CentOS Stream, which is "A preview of
> upcoming Red Hat Enterprise Linux minor and major releases.".
>
>  c) becase of b), it should be easier to identify and reduce the gap
> between Red Hat's downstream and upstream QEMU.
>
> The components themselves to achieve this custom job are:
>
>  1) build environment configuration: documentation and a playbook for
> a base Enterprise Linux 8 system (also applicable to CentOS
> Stream), which other users can run on their system to get the
> environment suitable for building QEMU.
>
>  2) QEMU build configuration: how QEMU will be built to match, as
> closely as possible, the binaries built and packaged on CentOS
> stream 8.
>
>  3) job definition: GitLab CI jobs that will dispatch the build/test
> job to the machine specifically configured according to #1.
>
> Signed-off-by: Cleber Rosa 
> ---
>  .gitlab-ci.d/custom-runners.yml|  29 
>  scripts/ci/org.centos/stream/README|   2 +
>  scripts/ci/org.centos/stream/configure | 190 +
>  scripts/ci/setup/build-environment.yml |  38 +
>  4 files changed, 259 insertions(+)
>  create mode 100644 scripts/ci/org.centos/stream/README
>  create mode 100755 scripts/ci/org.centos/stream/configure
>
> diff --git a/.gitlab-ci.d/custom-runners.yml b/.gitlab-ci.d/custom-runners.yml
> index 061d3cdfed..ee5143995e 100644
> --- a/.gitlab-ci.d/custom-runners.yml
> +++ b/.gitlab-ci.d/custom-runners.yml
> @@ -220,3 +220,32 @@ ubuntu-20.04-aarch64-notcg:
>   - ../configure --disable-libssh --disable-tcg
>   - make --output-sync -j`nproc`
>   - make --output-sync -j`nproc` check V=1
> +
> +centos-stream-8-x86_64:
> + allow_failure: true
> + needs: []
> + stage: build
> + tags:
> + - centos_stream_8
> + - x86_64
> + rules:
> + - if: '$CI_COMMIT_BRANCH =~ /^staging/'
> + artifacts:
> +   name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
> +   when: on_failure
> +   expire_in: 7 days
> +   paths:
> + - build/tests/results/latest/results.xml
> + - build/tests/results/latest/test-results
> +   reports:
> + junit: build/tests/results/latest/results.xml
> + script:
> + - mkdir build
> + - cd build
> + - ../scripts/ci/org.centos/stream/configure
> + - make --output-sync -j`nproc`
> + - make --output-sync -j`nproc` check V=1
> + - make get-vm-images
> + # Only run tests that are either marked explicitly for KVM and x86_64
> + # or tests that are supposed to be valid for all targets
> + - ./tests/venv/bin/avocado run --job-results-dir=tests/results/ 
> --filter-by-tags-include-empty --filter-by-tags-include-empty-key -t 
> accel:kvm,arch:x86_64 -- tests/acceptance/
> diff --git a/scripts/ci/org.centos/stream/README 
> b/scripts/ci/org.centos/stream/README
> new file mode 100644
> index 00..f99bda99b8
> --- /dev/null
> +++ b/scripts/ci/org.centos/stream/README
> @@ -0,0 +1,2 @@
> +This directory contains scripts for generating a build of QEMU that
> +closely matches the CentOS Stream builds of the qemu-kvm package.
> diff --git a/scripts/ci/org.centos/stream/configure 
> b/scripts/ci/org.centos/stream/configure
> new file mode 100755
> index 00..1e7207faec
> --- /dev/null
> +++ b/scripts/ci/org.centos/stream/configure
> @@ -0,0 +1,190 @@
> +#!/bin/sh -e
> +../configure \
> +--prefix="/usr" \
> +--libdir="/usr/lib64" \
> +--datadir="/usr/share" \
> +--sysconfdir="/etc" \
> +--interp-prefix=/usr/qemu-%M \
> +--localstatedir="/var" \
> +--docdir="/usr/share/doc" \
> +--libexecdir="/usr/libexec" \
> +--extra-ldflags="-Wl,--build-id -Wl,-z,relro -Wl,-z,now" \
> +--extra-cflags="-O2 -g -pipe -Wall -Werror=format-security 
> -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions 
> -fstack-protector-strong -grecord-gcc-switches 
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 
> -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic 
> -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection" \
> +--with-suffix="qemu-kvm" \
> +--firmwarepath=/usr/share/qemu-firmware \
> +--meson="/usr/bin/meson" \
> +--targe

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

[Cleber Rosa Junior]

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 
> > ---
> >   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 00..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 
> > +#
> > +# 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, descript

[PATCH 4/4] Jobs based on custom runners: add CentOS Stream 8

[Cleber Rosa]

This introduces three different parts of a job designed to run
on a custom runner managed by Red Hat.  The goals include:

 a) serve as a model for other organizations that want to onboard
their own runners, with their specific platforms, build
configuration and tests.

 b) bring awareness to the differences between upstream QEMU and the
version available under CentOS Stream, which is "A preview of
upcoming Red Hat Enterprise Linux minor and major releases.".

 c) becase of b), it should be easier to identify and reduce the gap
between Red Hat's downstream and upstream QEMU.

The components themselves to achieve this custom job are:

 1) build environment configuration: documentation and a playbook for
a base Enterprise Linux 8 system (also applicable to CentOS
Stream), which other users can run on their system to get the
environment suitable for building QEMU.

 2) QEMU build configuration: how QEMU will be built to match, as
closely as possible, the binaries built and packaged on CentOS
stream 8.

 3) job definition: GitLab CI jobs that will dispatch the build/test
job to the machine specifically configured according to #1.

Signed-off-by: Cleber Rosa 
---
 .gitlab-ci.d/custom-runners.yml|  29 
 scripts/ci/org.centos/stream/README|   2 +
 scripts/ci/org.centos/stream/configure | 190 +
 scripts/ci/setup/build-environment.yml |  38 +
 4 files changed, 259 insertions(+)
 create mode 100644 scripts/ci/org.centos/stream/README
 create mode 100755 scripts/ci/org.centos/stream/configure

diff --git a/.gitlab-ci.d/custom-runners.yml b/.gitlab-ci.d/custom-runners.yml
index 061d3cdfed..ee5143995e 100644
--- a/.gitlab-ci.d/custom-runners.yml
+++ b/.gitlab-ci.d/custom-runners.yml
@@ -220,3 +220,32 @@ ubuntu-20.04-aarch64-notcg:
  - ../configure --disable-libssh --disable-tcg
  - make --output-sync -j`nproc`
  - make --output-sync -j`nproc` check V=1
+
+centos-stream-8-x86_64:
+ allow_failure: true
+ needs: []
+ stage: build
+ tags:
+ - centos_stream_8
+ - x86_64
+ rules:
+ - if: '$CI_COMMIT_BRANCH =~ /^staging/'
+ artifacts:
+   name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
+   when: on_failure
+   expire_in: 7 days
+   paths:
+ - build/tests/results/latest/results.xml
+ - build/tests/results/latest/test-results
+   reports:
+ junit: build/tests/results/latest/results.xml
+ script:
+ - mkdir build
+ - cd build
+ - ../scripts/ci/org.centos/stream/configure
+ - make --output-sync -j`nproc`
+ - make --output-sync -j`nproc` check V=1
+ - make get-vm-images
+ # Only run tests that are either marked explicitly for KVM and x86_64
+ # or tests that are supposed to be valid for all targets
+ - ./tests/venv/bin/avocado run --job-results-dir=tests/results/ 
--filter-by-tags-include-empty --filter-by-tags-include-empty-key -t 
accel:kvm,arch:x86_64 -- tests/acceptance/
diff --git a/scripts/ci/org.centos/stream/README 
b/scripts/ci/org.centos/stream/README
new file mode 100644
index 00..f99bda99b8
--- /dev/null
+++ b/scripts/ci/org.centos/stream/README
@@ -0,0 +1,2 @@
+This directory contains scripts for generating a build of QEMU that
+closely matches the CentOS Stream builds of the qemu-kvm package.
diff --git a/scripts/ci/org.centos/stream/configure 
b/scripts/ci/org.centos/stream/configure
new file mode 100755
index 00..1e7207faec
--- /dev/null
+++ b/scripts/ci/org.centos/stream/configure
@@ -0,0 +1,190 @@
+#!/bin/sh -e
+../configure \
+--prefix="/usr" \
+--libdir="/usr/lib64" \
+--datadir="/usr/share" \
+--sysconfdir="/etc" \
+--interp-prefix=/usr/qemu-%M \
+--localstatedir="/var" \
+--docdir="/usr/share/doc" \
+--libexecdir="/usr/libexec" \
+--extra-ldflags="-Wl,--build-id -Wl,-z,relro -Wl,-z,now" \
+--extra-cflags="-O2 -g -pipe -Wall -Werror=format-security 
-Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions 
-fstack-protector-strong -grecord-gcc-switches 
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic 
-fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection" \
+--with-suffix="qemu-kvm" \
+--firmwarepath=/usr/share/qemu-firmware \
+--meson="/usr/bin/meson" \
+--target-list="x86_64-softmmu" \
+--block-drv-rw-whitelist=qcow2,raw,file,host_device,nbd,iscsi,rbd,blkdebug,luks,null-co,nvme,copy-on-read,throttle,gluster
 \
+--audio-drv-list= \
+--block-drv-ro-whitelist=vmdk,vhdx,vpc,https,ssh \
+--with-coroutine=ucontext \
+--with-git=git \
+--tls-priority=@QEMU,SYSTEM \
+--disable-attr \
+--disable-auth-pam \
+--disable-avx2 \
+--disable-avx512f \
+--disable-bochs \
+--disable-brlapi \
+--disable-bsd-user \
+--disable-bzip2 \
+--disable-cap-ng \
+--disable-capstone \
+--disable-cfi \
+--disable-cfi-debug \
+--disable-cloop \
+--disable-cocoa \
+--disable-coroutine-po

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

[Cleber Rosa]

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 
---
 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 00..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.
+#
+# Authors:
+#  Cleber Rosa 
+#
+# 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.
+
+@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:]]
-- 
2.25.4

[PATCH 3/4] Acceptance Tests: introduce method to require a feature and option

[Cleber Rosa]

In this context, and according to the qemu.utils.list_feature() utility
function, a feature is something is available as a QEMU command line
option that can take the "help" value.

This builds on top of that utility function, and allows test writers
to require, for instance, the "x-remote" (option) machine type
(feature).

This example is actually applied here to the reespective test, given
that the option is conditionally built, and the test will ERROR
without it.

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 29 ++-
 tests/acceptance/multiprocess.py  |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 93c4b9851f..432caff4e6 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -11,6 +11,7 @@
 import logging
 import os
 import shutil
+import subprocess
 import sys
 import uuid
 import tempfile
@@ -45,7 +46,8 @@
 from qemu.utils import (
 get_info_usernet_hostfwd_port,
 kvm_available,
-tcg_available,
+list_feature,
+tcg_available
 )
 
 def is_readable_executable_file(path):
@@ -182,6 +184,31 @@ def _get_unique_tag_val(self, tag_name):
 return vals.pop()
 return None
 
+def require_feature(self, feature, option=None):
+"""
+Requires a feature to be available for the test to continue
+
+It takes into account the currently set qemu binary, and only checks
+for by running a "qemu -$feature help" command.  If the specific option
+is given, it checks if it's listed for the given feature.
+
+If the check fails, the test is canceled.
+
+:param feature: name of a QEMU feature, such as "accel" or "machine"
+:type feature: str
+:param option: the specific value for the feature.  For instance,
+   if feature is "machine", option can be "q35".
+type option: str
+"""
+try:
+options_available = list_feature(self.qemu_bin, feature)
+except subprocess.CalledProcessError:
+self.cancel('Feature "%s" does not appear to be present.' % 
feature)
+if option is not None:
+if option not in options_available:
+self.cancel('Feature "%s" does not have "%s" as an option' %
+(feature, option))
+
 def require_accelerator(self, accelerator):
 """
 Requires an accelerator to be available for the test to continue
diff --git a/tests/acceptance/multiprocess.py b/tests/acceptance/multiprocess.py
index 96627f022a..4d8a40a510 100644
--- a/tests/acceptance/multiprocess.py
+++ b/tests/acceptance/multiprocess.py
@@ -22,6 +22,7 @@ def do_test(self, kernel_url, initrd_url, kernel_command_line,
 machine_type):
 """Main test method"""
 self.require_accelerator('kvm')
+self.require_feature('machine', 'x-remote')
 
 # Create socketpair to connect proxy and remote processes
 proxy_sock, remote_sock = socket.socketpair(socket.AF_UNIX,
-- 
2.25.4

[PATCH 1/4] block.c: fix compilation error on possible unitialized variable

[Cleber Rosa]

GCC from CentOS Stream 8 is erroring out on a possibily unitialized
varible.

Full version info for the compiler used:

 gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-1)

Signed-off-by: Cleber Rosa 
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3f456892d0..08f29e6b65 100644
--- a/block.c
+++ b/block.c
@@ -4866,7 +4866,7 @@ static int bdrv_replace_node_common(BlockDriverState 
*from,
 Transaction *tran = tran_new();
 g_autoptr(GHashTable) found = NULL;
 g_autoptr(GSList) refresh_list = NULL;
-BlockDriverState *to_cow_parent;
+BlockDriverState *to_cow_parent = NULL;
 int ret;
 
 if (detach_subchain) {
-- 
2.25.4

[PATCH 0/4] Jobs based on custom runners: add CentOS Stream 8

[Cleber Rosa]

This builds on top the "GitLab Custom Runners and Jobs (was: QEMU
Gating CI)" series, showing an example of how other entities can
add their own custom jobs to the GitLab CI pipeline.

First of all, it may be useful to see an actual pipeline (and the
reespective job introduced here) combined with the jobs introduced
on "GitLab Custom Runners and Jobs (was: QEMU Gating CI)":

 * https://gitlab.com/cleber.gnu/qemu/-/pipelines/316527166
 * https://gitlab.com/cleber.gnu/qemu/-/jobs/1325976765

The runner (the machine and job) is to be managed by Red Hat, and
adds, at the very least, bare metal x86_64 KVM testing capabilities to
the QEMU pipeline.  This brings extra coverage for some unittests, and
the ability to run the acceptance tests dependent on KVM.

The runner is already completely set up and registered to the
https://gitlab.com/qemu-project/qemu project instance, and jobs will
be triggered according to the same rules for the jobs introduced on
"GitLab Custom Runners and Jobs (was: QEMU Gating CI)", that is,
but pushes to the staging branch.  Still, the job is set with mode
"allow failures", so it should not disrupt the existing pipeline.
Once its reliability is proved (rules and service levels are to be
determined), that can be reverted.

Even though the formal method of tracking machine/job maintainers have
not been formalized, it should be known that the contacts/admins for
this machine and job are:

 - Cleber Rosa
   
   clebergnu on #qemu

 - Willian Rampazzo
   
   willianr on #qemu

Based-on: <20210608031425.833536-1-cr...@redhat.com>

Cleber Rosa (4):
  block.c: fix compilation error on possible unitialized variable
  Python QEMU utils: introduce a generic feature list
  Acceptance Tests: introduce method to require a feature and option
  Jobs based on custom runners: add CentOS Stream 8

 .gitlab-ci.d/custom-runners.yml   |  29 
 block.c   |   2 +-
 python/qemu/utils/__init__.py |   2 +
 python/qemu/utils/accel.py|  15 +-
 python/qemu/utils/feature.py  |  44 +
 scripts/ci/org.centos/stream/README   |   2 +
 scripts/ci/org.centos/stream/configure| 190 ++
 scripts/ci/setup/build-environment.yml|  38 +
 tests/acceptance/avocado_qemu/__init__.py |  29 +++-
 tests/acceptance/multiprocess.py  |   1 +
 10 files changed, 337 insertions(+), 15 deletions(-)
 create mode 100644 python/qemu/utils/feature.py
 create mode 100644 scripts/ci/org.centos/stream/README
 create mode 100755 scripts/ci/org.centos/stream/configure

-- 
2.25.4

Re: [PATCH v7 31/31] gitlab: add python linters to CI

[Cleber Rosa]

On Tue, May 25, 2021 at 08:24:54PM -0400, John Snow wrote:
> Add a python container that contains just enough juice for us to run the 
> python
> code quality analysis tools.
> 
> Base this container on fedora, because fedora has very convenient
> packaging for testing multiple python versions.
> 
> Add two tests:
> 
> check-python-pipenv uses pipenv to test a frozen, very explicit set of
> packages against our minimum supported python version, Python 3.6. This
> test is not allowed to fail.
> 
> check-python-tox uses tox to install the latest versions of required
> python dependencies against a wide array of Python versions from 3.6 to
> 3.9, even including the yet-to-be-released Python 3.10. This test is
> allowed to fail with a warning.
> 
> Signed-off-by: John Snow 
> ---
>  .gitlab-ci.d/containers.yml|  5 +
>  .gitlab-ci.yml | 26 ++
>  tests/docker/dockerfiles/python.docker | 18 ++
>  3 files changed, 49 insertions(+)
>  create mode 100644 tests/docker/dockerfiles/python.docker
> 
> diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
> index 765408ae274..05ebd4dc11d 100644
> --- a/.gitlab-ci.d/containers.yml
> +++ b/.gitlab-ci.d/containers.yml
> @@ -242,3 +242,8 @@ amd64-opensuse-leap-container:
>extends: .container_job_template
>variables:
>  NAME: opensuse-leap
> +
> +python-container:
> +  extends: .container_job_template
> +  variables:
> +NAME: python
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index f718b61fa78..cc2a3935c62 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -789,6 +789,32 @@ check-patch:
>  GIT_DEPTH: 1000

As others have pointed out, this can use a tweak.

>allow_failure: true
>
> +
> +check-python-pipenv:
> +  stage: test
> +  image: $CI_REGISTRY_IMAGE/qemu/python:latest
> +  script:
> +- cd python
> +- make venv-check

Nipick:

  - make -C python venv-check

> +  variables:
> +GIT_DEPTH: 1000
> +  needs:
> +job: python-container
> +
> +
> +check-python-tox:
> +  stage: test
> +  image: $CI_REGISTRY_IMAGE/qemu/python:latest
> +  script:
> +- cd python
> +- make check-tox
> +  variables:
> +GIT_DEPTH: 1000

Same here.

> +  needs:
> +job: python-container
> +  allow_failure: true
> +
> +
>  check-dco:
>stage: build
>image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
> diff --git a/tests/docker/dockerfiles/python.docker 
> b/tests/docker/dockerfiles/python.docker
> new file mode 100644
> index 000..56d88417df4
> --- /dev/null
> +++ b/tests/docker/dockerfiles/python.docker
> @@ -0,0 +1,18 @@
> +# Python library testing environment
> +
> +FROM fedora:latest
> +MAINTAINER John Snow 
> +
> +# Please keep this list sorted alphabetically
> +ENV PACKAGES \
> +gcc \

Now with this question answered, with or without the suggestion
above:

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v7 31/31] gitlab: add python linters to CI

[Cleber Rosa]

On Thu, May 27, 2021 at 12:17:36PM -0400, Cleber Rosa wrote:
> On Wed, May 26, 2021 at 03:56:31PM -0400, John Snow wrote:
> > On 5/26/21 2:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > > build requisite for PyPI packages in the event that PyPI only has a
> > > > sdist and not a bdist for a given dependency during installation.
> > > 
> > > i.e. some packages are compiled during installation?
> > 
> > Realized I didn't answer this directly. Yes, sometimes, depending on your
> > platform or your python version or how new the python package is, it may not
> > have a binary distribution available and will require compilation.
> >
> 
> But shouldn't this be known at this time, given that you're putting
> the depedencies for one specific platform?  I'd very very much like
> to know which packages, for this specific platform, is triggering
> a Python package build that has C-based extensions.
> 
> And it would be even weired if such a package does *not* have C-based
> extensions, and it's still requiring gcc.  I would judge it as a
> major setuptools design issue.
> 
> > This comes up for Python 3.10 dependencies right now in particular. They do
> > not have binary distributions because (I assume) 3.10 isn't finalized yet,
> > so they haven't done a re-build. Or something like that.
> > 
> > --js
> 
> OK... but can you share which package available only in source is
> requiring gcc?  I'm not going to get a good night of sleep without
> knowing that! :)
> 
> Thanks,
> - Cleber.

OK, so typed-ast is the culprit, and we can attest its requirement
for a compiler here:

  
https://github.com/python/typed_ast/blob/8eed936014f81a55a3e17310629c40c0203327c3/setup.py#L123

Now I can sleep in peace. :)

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v7 17/31] python: add pylint to pipenv

[Cleber Rosa]

On Tue, May 25, 2021 at 08:24:40PM -0400, John Snow wrote:
> We are specifying >= pylint 2.8.x for several reasons:
> 
> 1. For setup.cfg support, added in pylint 2.5.x
> 2. To specify a version that has incompatibly dropped
>bad-whitespace checks (2.6.x)
> 3. 2.7.x fixes "unsubscriptable" warnings in Python 3.9
> 4. 2.8.x adds a new, incompatible 'consider-using-with'
>warning that must be disabled in some cases.
>These pragmas cause warnings themselves in 2.7.x.
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile  |   1 +
>  python/Pipfile.lock | 130 
>  2 files changed, 131 insertions(+)
>  create mode 100644 python/Pipfile.lock
>

pylint bump successful:

   $ pipenv run pylint --version
   pylint 2.8.2
   astroid 2.5.6
   Python 3.6.12 (default, Aug 19 2020, 00:00:00) 
   [GCC 9.3.1 20200408 (Red Hat 9.3.1-2)]

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v7 13/31] python: add MANIFEST.in

[Cleber Rosa]

On Tue, May 25, 2021 at 08:24:36PM -0400, John Snow wrote:
> When creating a source or binary distribution via 'python3 setup.py
> ', the VERSION and PACKAGE.rst files aren't bundled by
> default. Create a MANIFEST.in file that instructs the build tools to
> include these so that installation from these files won't fail.
> 
> This is required by 'tox', as well as by the tooling needed to upload
> packages to PyPI.
> 
> Exclude the 'README.rst' file -- that's intended as a guidebook to our
> source tree, not a file that needs to be distributed.
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst  | 2 ++
>  python/MANIFEST.in | 3 +++
>  2 files changed, 5 insertions(+)
>  create mode 100644 python/MANIFEST.in
>

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v7 09/31] python: create qemu packages

[Cleber Rosa]

On Tue, May 25, 2021 at 08:24:32PM -0400, John Snow wrote:
> move python/qemu/*.py to python/qemu/[machine, qmp, utils]/*.py and
> update import directives across the tree.
> 
> This is done to create a PEP420 namespace package, in which we may
> create subpackages. To do this, the namespace directory ("qemu") should
> not have any modules in it. Those files will go into new 'machine',
> 'qmp' and 'utils' subpackages instead.
> 
> Implement machine/__init__.py making the top-level classes and functions
> from its various modules available directly inside the package. Change
> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> library classes are available directly from "qemu.qmp" instead of
> "qemu.qmp.qmp".
> 
> Signed-off-by: John Snow 
> ---
>  python/{qemu => }/.isort.cfg|  0
>  python/qemu/__init__.py | 11 ---
>  python/qemu/{ => machine}/.flake8   |  0
>  python/qemu/machine/__init__.py | 33 +
>  python/qemu/{ => machine}/console_socket.py |  0
>  python/qemu/{ => machine}/machine.py| 16 ++
>  python/qemu/{ => machine}/pylintrc  |  0
>  python/qemu/{ => machine}/qtest.py  |  3 +-
>  python/qemu/{qmp.py => qmp/__init__.py} | 12 +++-
>  python/qemu/{utils.py => utils/__init__.py} | 18 +--
>  python/qemu/{ => utils}/accel.py|  0
>  tests/acceptance/avocado_qemu/__init__.py   |  9 +++---
>  tests/acceptance/virtio-gpu.py  |  2 +-
>  tests/qemu-iotests/300  |  4 +--
>  tests/qemu-iotests/iotests.py   |  2 +-
>  tests/vm/aarch64vm.py   |  2 +-
>  tests/vm/basevm.py  |  3 +-
>  17 files changed, 83 insertions(+), 32 deletions(-)
>  rename python/{qemu => }/.isort.cfg (100%)
>  delete mode 100644 python/qemu/__init__.py
>  rename python/qemu/{ => machine}/.flake8 (100%)
>  create mode 100644 python/qemu/machine/__init__.py
>  rename python/qemu/{ => machine}/console_socket.py (100%)
>  rename python/qemu/{ => machine}/machine.py (98%)
>  rename python/qemu/{ => machine}/pylintrc (100%)
>  rename python/qemu/{ => machine}/qtest.py (99%)
>  rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
>  rename python/qemu/{utils.py => utils/__init__.py} (66%)
>  rename python/qemu/{ => utils}/accel.py (100%)
>

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v7 10/31] python: add qemu package installer

[Cleber Rosa]

On Tue, May 25, 2021 at 08:24:33PM -0400, John Snow wrote:
> Add setup.cfg and setup.py, necessary for installing a package via
> pip. Add a ReST document (PACKAGE.rst) explaining the basics of what
> this package is for and who to contact for more information. This
> document will be used as the landing page for the package on PyPI.
> 
> List the subpackages we intend to package by name instead of using
> find_namespace because find_namespace will naively also packages tests,
> things it finds in the dist/ folder, etc. I could not figure out how to
> modify this behavior; adding allow/deny lists to setuptools kept
> changing the packaged hierarchy. This works, roll with it.
> 
> I am not yet using a pyproject.toml style package manifest, because
> "editable" installs are not defined (yet?) by PEP-517/518.
> 
> I consider editable installs crucial for development, though they have
> (apparently) always been somewhat poorly defined.
> 
> Pip now (19.2 and later) now supports editable installs for projects
> using pyproject.toml manifests, but might require the use of the
> --no-use-pep517 flag, which somewhat defeats the point. Full support for
> setup.py-less editable installs was not introduced until pip 21.1.1:
> https://github.com/pypa/pip/pull/9547/commits/7a95720e796a5e56481c1cc20b6ce6249c50f357
> 
> For now, while the dust settles, stick with the de-facto
> setup.py/setup.cfg combination supported by setuptools. It will be worth
> re-evaluating this point again in the future when our supported build
> platforms all ship a fairly modern pip.
> 
> Additional reading on this matter:
> 
> https://github.com/pypa/packaging-problems/issues/256
> https://github.com/pypa/pip/issues/6334
> https://github.com/pypa/pip/issues/6375
> https://github.com/pypa/pip/issues/6434
> https://github.com/pypa/pip/issues/6438
> 
> Signed-off-by: John Snow 
> ---
>  python/PACKAGE.rst | 33 +
>  python/setup.cfg   | 22 ++
>  python/setup.py| 23 +++
>  3 files changed, 78 insertions(+)
>  create mode 100644 python/PACKAGE.rst
>  create mode 100644 python/setup.cfg
>  create mode 100755 python/setup.py
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> new file mode 100644
> index 000..1bbfe1b58e2
> --- /dev/null
> +++ b/python/PACKAGE.rst
> @@ -0,0 +1,33 @@
> +QEMU Python Tooling
> +===
> +
> +This package provides QEMU tooling used by the QEMU project to build,
> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
> +to change at any time.
> +
> +Usage
> +-
> +
> +The ``qemu.qmp`` subpackage provides a library for communicating with
> +QMP servers. The ``qemu.machine`` subpackage offers rudimentary
> +facilities for launching and managing QEMU processes. Refer to each
> +package's documentation
> +(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
> +for more information.
> +
> +Contributing
> +
> +
> +This package is maintained by John Snow  as part of
> +the QEMU source tree. Contributions are welcome and follow the `QEMU
> +patch submission process
> +<https://wiki.qemu.org/Contribute/SubmitAPatch>`_, which involves
> +sending patches to the QEMU development mailing list.
> +
> +John maintains a `GitLab staging branch
> +<https://gitlab.com/jsnow/qemu/-/tree/python>`_, and there is an
> +official `GitLab mirror <https://gitlab.com/qemu-project/qemu>`_.
> +
> +Please report bugs on the `QEMU issue tracker
> +<https://gitlab.com/qemu-project/qemu/-/issues>`_ and tag ``@jsnow`` in
> +the report.
> diff --git a/python/setup.cfg b/python/setup.cfg
> new file mode 100644
> index 000..3fa92a2e73f
> --- /dev/null
> +++ b/python/setup.cfg
> @@ -0,0 +1,22 @@
> +[metadata]
> +name = qemu
> +maintainer = QEMU Developer Team
> +maintainer_email = qemu-de...@nongnu.org
> +url = https://www.qemu.org/
> +download_url = https://www.qemu.org/download/
> +description = QEMU Python Build, Debug and SDK tooling.
> +long_description = file:PACKAGE.rst
> +long_description_content_type = text/x-rst
> +classifiers =
> +Development Status :: 3 - Alpha
> +License :: OSI Approved :: GNU General Public License v2 (GPLv2)
> +Natural Language :: English
> +Operating System :: OS Independent
> +Programming Language :: Python :: 3 :: Only
> +
> +[options]
> +python_requires = >= 3.6
> +packages =
> +qemu.qmp
> +qemu.machine
> +qemu.utils

^ Good!

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v7 31/31] gitlab: add python linters to CI

[Cleber Rosa]

On Wed, May 26, 2021 at 03:56:31PM -0400, John Snow wrote:
> On 5/26/21 2:47 PM, Vladimir Sementsov-Ogievskiy wrote:
> > > build requisite for PyPI packages in the event that PyPI only has a
> > > sdist and not a bdist for a given dependency during installation.
> > 
> > i.e. some packages are compiled during installation?
> 
> Realized I didn't answer this directly. Yes, sometimes, depending on your
> platform or your python version or how new the python package is, it may not
> have a binary distribution available and will require compilation.
>

But shouldn't this be known at this time, given that you're putting
the depedencies for one specific platform?  I'd very very much like
to know which packages, for this specific platform, is triggering
a Python package build that has C-based extensions.

And it would be even weired if such a package does *not* have C-based
extensions, and it's still requiring gcc.  I would judge it as a
major setuptools design issue.

> This comes up for Python 3.10 dependencies right now in particular. They do
> not have binary distributions because (I assume) 3.10 isn't finalized yet,
> so they haven't done a re-build. Or something like that.
> 
> --js

OK... but can you share which package available only in source is
requiring gcc?  I'm not going to get a good night of sleep without
knowing that! :)

Thanks,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v7 07/31] python/machine: Trim line length to below 80 chars

[Cleber Rosa]

On Tue, May 25, 2021 at 08:24:30PM -0400, John Snow wrote:
> One more little delinting fix that snuck in.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v7 29/31] python: add .gitignore

[Cleber Rosa]

On Wed, May 26, 2021 at 12:18:55PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 26.05.2021 03:24, John Snow wrote:
> > Ignore *Python* build and package output (build, dist, qemu.egg-info);
> > these files are not created as part of a QEMU build. They are created by
> > running the commands 'python3 setup.py ' when preparing
> > tarballs to upload to e.g. PyPI.
> > 
> > Ignore miscellaneous cached python confetti (mypy, pylint, et al)
> > 
> > Ignore .idea (pycharm) .vscode, and .venv (pipenv et al).
> > 
> > Signed-off-by: John Snow 
> > ---
> >   python/.gitignore | 15 +++
> >   1 file changed, 15 insertions(+)
> >   create mode 100644 python/.gitignore

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 23/25] python: add .gitignore

[Cleber Rosa]

On Tue, May 25, 2021 at 04:10:55PM -0400, John Snow wrote:
> On 5/25/21 3:36 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:39PM -0400, John Snow wrote:
> > > Ignore *Python* build and package output (build, dist, qemu.egg-info);
> > > these files are not created as part of a QEMU build.
> > > 
> > > Ignore miscellaneous cached python confetti (__pycache__, *.pyc,
> > > .mypy_cache).
> > > 
> > > Ignore .idea (pycharm) .vscode, and .venv (pipenv et al).
> > > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/.gitignore | 19 +++
> > >   1 file changed, 19 insertions(+)
> > >   create mode 100644 python/.gitignore
> > > 
> > > diff --git a/python/.gitignore b/python/.gitignore
> > > new file mode 100644
> > > index 000..e27c99e009c
> > > --- /dev/null
> > > +++ b/python/.gitignore
> > > @@ -0,0 +1,19 @@
> > > +# python bytecode cache
> > > +*.pyc
> > 
> > This is a duplicate from the parent .gitignore, so I would avoid it.
> > 
> > > +__pycache__/
> > 
> > And this one is interesting because, the only thing that *should* be
> > in __pycache__ dirs is .pyc files (covered by the parent .gitignore
> > file).
> > 
> > So, I get the same behavior without these two entries here, so I would
> > skip them.  Let me know if you have any reason for explicitly
> > including them.
> > 
> > - Cleber.
> > 
> 
> Hm, not really ... Just completeness, I suppose, since this directory is
> becoming increasingly separate from the rest of the tree.
> 
> It isn't crucial, it just seemed like a weird omission if they weren't
> listed here. *shrug*
> 
> --js

And still, this dir is part of the overall tree.  Honestly, without
any change in behavior, I'd *not* add those two ignore rules.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v6 25/25] python: add tox support

[Cleber Rosa]

On Tue, May 25, 2021 at 04:25:37PM -0400, John Snow wrote:
> On 5/25/21 4:15 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:41PM -0400, John Snow wrote:
> > > This is intended to be a manually run, non-CI script.
> > > 
> > > Use tox to test the linters against all python versions from 3.6 to
> > > 3.9. This will only work if you actually have those versions installed
> > > locally, but Fedora makes this easy:
> > > 
> > > > sudo dnf install python36 python37 python38 python39
> > > 
> > > Unlike the pipenv tests (make venv-check), this pulls "whichever"
> > > versions of the python packages, so they are unpinned and may break as
> > > time goes on. In the case that breakages are found, setup.cfg should be
> > > amended accordingly to avoid the bad dependant versions, or the code
> > > should be amended to work around the issue.
> > > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/README.rst |  2 ++
> > >   python/.gitignore |  1 +
> > >   python/Makefile   |  7 ++-
> > >   python/setup.cfg  |  1 +
> > >   python/tox.ini| 13 +
> > >   5 files changed, 23 insertions(+), 1 deletion(-)
> > >   create mode 100644 python/tox.ini
> > > 
> > 
> > This works as intended for me.  A couple of notes / suggestions
> > for future improvements:
> > 
> >   * `dnf install tox` pulled all the Python versions available (I
> > assume as suggestions) automatically
> > 
> >   * tox.ini can be folded into setup.cfg
> > 
> 
> Done!
>

Nice!

> >   * a custom container image with all those Python versions may be
> > handy for running both the pipenv based job (along with the
> > suggestions on the previous patch) and an on-demand,
> > "allow_failure" tox based CI job.
> > 
> 
> Yeah, I was thinking this would be good, too!
> 
> I think at this point, it's going to be a follow-up, though. Because
> ideally, yes, this SHOULD pass -- it's just that it needs a fairly
> particular environment to run in, which is annoying, so it's here as an
> optional-ish thing for now.
> 
> Maybe I'll make a new fedora:latest container that's meant solely for
> testing python stuff, because it's just such a convenient distro for it.
> 
> Later, though.
>

Sure.

> > Other than those suggestions, this LGTM!
> > 
> > Reviewed-by: Cleber Rosa 
> > Tested-by: Cleber Rosa 
> > 
> 
> 🎉

\o/ (with 3 characters, because I'm unable to find the right codepoint)


signature.asc
Description: PGP signature

Re: [PATCH v6 22/25] python: add Makefile for some common tasks

[Cleber Rosa]

On Tue, May 25, 2021 at 03:45:26PM -0400, John Snow wrote:
> On 5/25/21 3:24 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:38PM -0400, John Snow wrote:
> > > Add "make venv" to create the pipenv-managed virtual environment that
> > > contains our explicitly pinned dependencies.
> > > 
> > > Add "make check" to run the python linters [in the host execution
> > > environment].
> > > 
> > > Add "make venv-check" which combines the above two: create/update the
> > > venv, then run the linters in that explicitly managed environment.
> > > 
> > > Add "make develop" which canonizes the runes needed to get both the
> > > linting pre-requisites (the "[devel]" part), and the editable
> > > live-install (the "-e" part) of these python libraries.
> > > 
> > > make clean: delete miscellaneous python packaging output possibly
> > > created by pipenv, pip, or other python packaging utilities
> > > 
> > > make distclean: delete the above, the .venv, and the editable "qemu"
> > > package forwarder (qemu.egg-info) if there is one.
> > > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/README.rst |  3 +++
> > >   python/Makefile   | 42 ++
> > >   2 files changed, 45 insertions(+)
> > >   create mode 100644 python/Makefile
> > > 
> > > diff --git a/python/README.rst b/python/README.rst
> > > index e107bd12a69..3e09d20c23c 100644
> > > --- a/python/README.rst
> > > +++ b/python/README.rst
> > > @@ -35,6 +35,9 @@ Files in this directory
> > >   - ``qemu/`` Python package source directory.
> > >   - ``tests/`` Python package tests directory.
> > >   - ``avocado.cfg`` Configuration for the Avocado test-runner.
> > > +  Used by ``make check`` et al.
> > > +- ``Makefile`` provides some common testing/installation invocations.
> > > +  Try ``make help`` to see available targets.
> > >   - ``MANIFEST.in`` is read by python setuptools, it specifies additional 
> > > files
> > > that should be included by a source distribution.
> > >   - ``PACKAGE.rst`` is used as the README file that is visible on 
> > > PyPI.org.
> > > diff --git a/python/Makefile b/python/Makefile
> > > new file mode 100644
> > > index 000..184f59e5634
> > > --- /dev/null
> > > +++ b/python/Makefile
> > > @@ -0,0 +1,42 @@
> > > +.PHONY: help venv venv-check check clean distclean develop
> > > +
> > > +help:
> > > + @echo "python packaging help:"
> > > + @echo ""
> > > + @echo "make venv:   Create pipenv's virtual environment."
> > > + @echo "NOTE: Requires Python 3.6 and pipenv."
> > > + @echo "  Will download packages from PyPI."
> > > + @echo "Hint: (On Fedora): 'sudo dnf install python36 pipenv'"
> > > + @echo ""
> > > + @echo "make venv-check: run linters using pipenv's virtual environment."
> > > + @echo "Hint: If you don't know which test to run, run this one!"
> > > + @echo ""
> > > + @echo "make develop:Install deps for 'make check', and"
> > > + @echo " the qemu libs in editable/development mode."
> > > + @echo ""
> > > + @echo "make check:  run linters using the current environment."
> > > + @echo ""
> > 
> > Let's observe how this will be used (or misused).  I fear most people
> > will jump into `make check`, even though you have described `make
> > venv-check` as the primary choice.
> > 
> > We have a precedent with `make check-acceptance` that will create a
> > venv and use it by default, so we can consider that as a fallback
> > strategy based on user feedback.
> > 
> 
> Right, I see. Though, I did intentionally want to make it clear which of
> these invocations created an environment and which did not.
> 
> Unlike the acceptance tests, it might make sense to run these tests both
> inside and outside of that venv, so I opted to make the default "make"
> target "make help".
> 
> The Gitlab CI will run the right one, after all -- and I do still expect the
> regular 'make check' to pass, so I am not as sure that it's a crucial
> failure if someone runs the "wrong one".
> 
> > > + @echo "make clean:  remove build output."
> > > + @echo ""
> > > + @echo "make distclean:  remove venv files, qemu package forwarder, and"
> > > + @echo " everything from 'make clean'."
> > > +
> > > +venv: .venv
> > > +.venv: Pipfile.lock
> > > + @PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
> > > + @touch .venv
> > > +
> > > +venv-check: venv
> > > + @pipenv run make check
> > > +
> > > +develop:
> > > + pip3 install -e .[devel]
> > > +
> > > +check:
> > > + @avocado --config avocado.cfg run tests/
> > > +
> > > +clean:
> > > + rm -rf build/ dist/
> > > +
> > 
> > Usually `python3 setup.py clean --all` would be the better choice here,
> > but, it doesn't clean `dist/`, so I'm OK with this.
> > 
> 
> Hm, I should probably move the 'dist' down into 'distclean' anyway, and I
> will replace the clean invocation with the one you suggest.
>

OK then, makes sense to me.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v6 20/25] python: add devel package requirements to setuptools

[Cleber Rosa]

On Tue, May 25, 2021 at 01:43:42PM -0400, John Snow wrote:
> On 5/25/21 12:13 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:36PM -0400, John Snow wrote:
> > > setuptools doesn't have a formal understanding of development requires,
> > > but it has an optional feataures section. Fine; add a "devel" feature
> > > and add the requirements to it.
> > > 
> > > To avoid duplication, we can modify pipenv to install qemu[devel]
> > > instead. This enables us to run invocations like "pip install -e
> > > .[devel]" and test the package on bleeding-edge packages beyond those
> > > specified in Pipfile.lock.
> > > 
> > > Importantly, this also allows us to install the qemu development
> > > packages in a non-networked mode: `pip3 install --no-index -e .[devel]`
> > > will now fail if the proper development dependencies are not already
> > > met. This can be useful for automated build scripts where fetching
> > > network packages may be undesirable.
> > > 
> > 
> > This is a fairly exotic feature of setuptools, with very very few
> > packages that I know about using it.  With most users (I believe)
> > relying on pipenv to get the exact packages, the setuptools/pip use
> > case may fall into obscurity IMO.
> > 
> 
> Fair enough.
> 
> The intent is:
> 
> - Pipenv is more for CI, to deploy a consistent set of frozen packages that
> are known to behave in an extremely stable manner. My hope is to avoid
> breaking changes introduced unknowingly by pylint et al.
> 
> - pip install qemu[devel] is intended more for external/normal use by
> developers. It grabs the latest and greatest and it may indeed break as
> dependencies change beyond my awareness.
> 
> 
> Some packages like aiohttp use that optional dependency feature to install
> optional modules -- `pip install aiohttp[speedups]` installs optional
> dependencies that allow that module to work much faster, but aren't
> required.
> 
> Since these linting tools aren't *required* just to *use* the package, I am
> doing users a courtesy by listing them as optional. That way, they aren't
> pulled in when using "pip install qemu", and if I have to pin on specific
> sub-versions etc, it won't include conflict dependencies for people using
> other projects that DO declare a hard requirement on those packages.
> 
> I can amend the PACKAGE.rst file to mention this usage, though it's only
> useful for folks developing the package.
> 
> (Still, part of the ploy here is to attract outside help on developing the
> QEMU SDK, pull requests welcome etc, so it's worth a documentation blurb for
> now.)
>

Yep, I agree with your reasoning here.  I just felt like an extra bit
of documentation would do the trick.

> > So my suggestion is: consider better exposing the fact that this is
> > available (a documentation section perhaps).
> > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/Pipfile  |  5 +
> > >   python/Pipfile.lock | 14 +-
> > >   python/setup.cfg|  9 +
> > >   3 files changed, 19 insertions(+), 9 deletions(-)
> > > 
> > 
> > Either way,
> > 
> > Reviewed-by: Cleber Rosa 
> > 
> 
> Thanks! I am taking your R-B and I have applied the following diff.
> 
> Note that the PACKAGE.rst blurb references qemu[devel] instead because the
> PACKAGE.rst file is what is displayed theoretically on PyPI. That exact
> invocation will fail currently, because it's not on PyPI yet.
> 
> A little weird, but I *think* it's correct.
> 
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> index 1bbfe1b58e2..05ea7789fc1 100644
> --- a/python/PACKAGE.rst
> +++ b/python/PACKAGE.rst
> @@ -31,3 +31,7 @@ official `GitLab mirror
> <https://gitlab.com/qemu-project/qemu>`_.
>  Please report bugs on the `QEMU issue tracker
>  <https://gitlab.com/qemu-project/qemu/-/issues>`_ and tag ``@jsnow`` in
>  the report.
> +
> +Optional packages necessary for running code quality analysis for this
> +package can be installed with the optional dependency group "devel":
> +``pip install qemu[devel]``.
> diff --git a/python/README.rst b/python/README.rst
> index bf9bbca979a..954870973d0 100644
> --- a/python/README.rst
> +++ b/python/README.rst
> @@ -24,6 +24,10 @@ which installs a version of the package that installs a
> forwarder
>  pointing to these files, such that the package always reflects the
>  latest version in your git tree.
> 
> +Installing ".[devel]" instead of "." will additionally pull in required
> +packages for testing this package. They are not runtime requirements,
> +and are not needed to simply use these libraries.
> +
>  See `Installing packages using pip and virtual environments
> 
> <https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
>  for more information.

Looks great to me.  And, let me be clear about it:

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 18/25] python/qemu: add isort to pipenv

[Cleber Rosa]

On Tue, May 25, 2021 at 01:21:25PM -0400, John Snow wrote:
> On 5/25/21 11:56 AM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:34PM -0400, John Snow wrote:
> > > isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
> > > certain "from ..." clauses that are not related to imports.
> > > 
> > > isort < 5.1.1 has a bug where it does not handle comments near import
> > > statements correctly.
> > > 
> > > Require 5.1.2 or greater.
> > > 
> > > isort can be run with 'isort -c qemu' from the python root.
> > > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/Pipfile  | 1 +
> > >   python/Pipfile.lock | 4 ++--
> > >   2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > Reviewed-by: Cleber Rosa 
> > 
> 
> Thanks. I have also updated the commit message a little bit:
> 
> isort can be run (in "check" mode) with 'isort -c qemu' from the python
> root. isort can also be used to fix/rewrite import order automatically
> by using 'isort qemu'.
> 
> --js

OK, sounds even better!

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v6 06/25] python: add directory structure README.rst files

[Cleber Rosa]

On Tue, May 25, 2021 at 01:14:50PM -0400, John Snow wrote:
> On 5/24/21 10:33 PM, Cleber Rosa wrote:
> > On Wed, May 12, 2021 at 07:12:22PM -0400, John Snow wrote:
> > > Add short readmes to python/, python/qemu/, python/qemu/machine,
> > > python/qemu/qmp, and python/qemu/utils that explain the directory
> > > hierarchy. These readmes are visible when browsing the source on
> > > e.g. gitlab/github and are designed to help new developers/users quickly
> > > make sense of the source tree.
> > > 
> > > They are not designed for inclusion in a published manual.
> > > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/README.rst  | 41 ++
> > >   python/qemu/README.rst |  8 +++
> > >   python/qemu/machine/README.rst |  9 
> > >   python/qemu/qmp/README.rst |  9 
> > >   python/qemu/utils/README.rst   |  7 ++
> > >   5 files changed, 74 insertions(+)
> > >   create mode 100644 python/README.rst
> > >   create mode 100644 python/qemu/README.rst
> > >   create mode 100644 python/qemu/machine/README.rst
> > >   create mode 100644 python/qemu/qmp/README.rst
> > >   create mode 100644 python/qemu/utils/README.rst
> > > 
> > > diff --git a/python/README.rst b/python/README.rst
> > > new file mode 100644
> > > index 000..7a0dc5dff4a
> > > --- /dev/null
> > > +++ b/python/README.rst
> > > @@ -0,0 +1,41 @@
> > > +QEMU Python Tooling
> > > +===
> > > +
> > > +This directory houses Python tooling used by the QEMU project to build,
> > > +configure, and test QEMU. It is organized by namespace (``qemu``), and
> > > +then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
> > > +
> > > +``setup.py`` is used by ``pip`` to install this tooling to the current
> > > +environment. ``setup.cfg`` provides the packaging configuration used by
> > > +setup.py in a setuptools specific format. You will generally invoke it
> > 
> > For consistency, ``setup.py`` here?  Also, I guess ``setuptools`` as it
> > falls in the same category of ``pip``.
> > 
> 
> Kinda-sorta, but `pip` is a command line executable and setuptools isn't.
> Along those lines I'll fix setup.py but leave setuptools as-is.
>

OK, fair enough.

> > > +by doing one of the following:
> > > +
> > > +1. ``pip3 install .`` will install these packages to your current
> > > +   environment. If you are inside a virtual environment, they will
> > > +   install there. If you are not, it will attempt to install to the
> > > +   global environment, which is not recommended.
> > 
> > Maybe some **emphasis** on **not**?
> > 
> 
> Sure :)
> 
> > > +
> > > +2. ``pip3 install --user .`` will install these packages to your user's
> > > +   local python packages. If you are inside of a virtual environment,
> > > +   this will fail.
> > > +
> > 
> > Maybe note that, if you are inside of a virtual environment, option #1
> > will probably be what users doing "--user" in a venv actually want.
> > 
> 
> Yes. It's frequently annoying how this works, because it's hard to relay
> succinctly :)
> 
> I think at least newer versions of pip give you good warnings when you use
> --user for virtual environments at least.
>

True.

> > > +If you append the ``-e`` argument, pip will install in "editable" mode;
> > > +which installs a version of the package that installs a forwarder
> > > +pointing to these files, such that the package always reflects the
> > > +latest version in your git tree.
> > > +
> > > +See `Installing packages using pip and virtual environments
> > > +<https://packaging.python.org/guides/installing-using-pip-and-virtual-environments/>`_
> > > +for more information.
> > > +
> > > +
> > > +Files in this directory
> > > +---
> > > +
> > > +- ``qemu/`` Python package source directory.
> > > +- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
> > > +- ``README.rst`` you are here!
> > > +- ``VERSION`` contains the PEP-440 compliant version used to describe
> > > +  this package; it is referenced by ``setup.cfg``.
> > > +- ``setup.cfg`` houses setuptools package configuration.
> > > +- ``setup.py`` is the setuptools installer used by pip; See above.
> &g

Re: [PATCH v6 25/25] python: add tox support

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:41PM -0400, John Snow wrote:
> This is intended to be a manually run, non-CI script.
> 
> Use tox to test the linters against all python versions from 3.6 to
> 3.9. This will only work if you actually have those versions installed
> locally, but Fedora makes this easy:
> 
> > sudo dnf install python36 python37 python38 python39
> 
> Unlike the pipenv tests (make venv-check), this pulls "whichever"
> versions of the python packages, so they are unpinned and may break as
> time goes on. In the case that breakages are found, setup.cfg should be
> amended accordingly to avoid the bad dependant versions, or the code
> should be amended to work around the issue.
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst |  2 ++
>  python/.gitignore |  1 +
>  python/Makefile   |  7 ++-
>  python/setup.cfg  |  1 +
>  python/tox.ini| 13 +
>  5 files changed, 23 insertions(+), 1 deletion(-)
>  create mode 100644 python/tox.ini
> 

This works as intended for me.  A couple of notes / suggestions
for future improvements:

 * `dnf install tox` pulled all the Python versions available (I
   assume as suggestions) automatically

 * tox.ini can be folded into setup.cfg

 * a custom container image with all those Python versions may be
   handy for running both the pipenv based job (along with the
   suggestions on the previous patch) and an on-demand,
   "allow_failure" tox based CI job.

Other than those suggestions, this LGTM!

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 24/25] gitlab: add python linters to CI

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:40PM -0400, John Snow wrote:
> Add python3.6 to the fedora container image: we need it to run the
> linters against that explicit version to make sure we don't break our
> minimum version promise.
> 
> Add pipenv so that we can fetch precise versions of pip packages we need
> to guarantee test reproducability.
> 
> Signed-off-by: John Snow 
> ---
>  .gitlab-ci.yml | 12 
>  tests/docker/dockerfiles/fedora.docker |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index dcb6317aace..a371c0c7163 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -779,6 +779,18 @@ check-patch:
>  GIT_DEPTH: 1000
>allow_failure: true
>  
> +
> +check-python:
> +  stage: test
> +  image: $CI_REGISTRY_IMAGE/qemu/fedora:latest
> +  script:
> +- cd python
> +- make venv-check
> +  variables:
> +GIT_DEPTH: 1000
> +  needs:
> +job: amd64-fedora-container
> +
>  check-dco:
>stage: build
>image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
> diff --git a/tests/docker/dockerfiles/fedora.docker 
> b/tests/docker/dockerfiles/fedora.docker
> index 915fdc1845e..6908d69ac37 100644
> --- a/tests/docker/dockerfiles/fedora.docker
> +++ b/tests/docker/dockerfiles/fedora.docker
> @@ -84,6 +84,7 @@ ENV PACKAGES \
>  numactl-devel \
>  perl \
>  perl-Test-Harness \
> +pipenv \
>  pixman-devel \
>  python3 \
>  python3-PyYAML \
> @@ -93,6 +94,7 @@ ENV PACKAGES \
>  python3-pip \
>  python3-sphinx \
>  python3-virtualenv \
> +python3.6 \

I personally would prefer having a different container image for this
job.  Because it would:

1. Be super simple (FROM fedora:33 / dnf -y install python3.6 pipenv)
2. Not carry all this unnecessary baggage
3. Not risk building QEMU with Python 3.6 (suppose the ./configure
   probe changes unintentionally)

But, AFAICT there is no precedent in requiring new images for
different types of checks.  So, unless someone else complains loudly,
I'm OK with this.

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 23/25] python: add .gitignore

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:39PM -0400, John Snow wrote:
> Ignore *Python* build and package output (build, dist, qemu.egg-info);
> these files are not created as part of a QEMU build.
> 
> Ignore miscellaneous cached python confetti (__pycache__, *.pyc,
> .mypy_cache).
> 
> Ignore .idea (pycharm) .vscode, and .venv (pipenv et al).
> 
> Signed-off-by: John Snow 
> ---
>  python/.gitignore | 19 +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 python/.gitignore
> 
> diff --git a/python/.gitignore b/python/.gitignore
> new file mode 100644
> index 000..e27c99e009c
> --- /dev/null
> +++ b/python/.gitignore
> @@ -0,0 +1,19 @@
> +# python bytecode cache
> +*.pyc

This is a duplicate from the parent .gitignore, so I would avoid it.

> +__pycache__/

And this one is interesting because, the only thing that *should* be
in __pycache__ dirs is .pyc files (covered by the parent .gitignore
file).

So, I get the same behavior without these two entries here, so I would
skip them.  Let me know if you have any reason for explicitly
including them.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v6 22/25] python: add Makefile for some common tasks

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:38PM -0400, John Snow wrote:
> Add "make venv" to create the pipenv-managed virtual environment that
> contains our explicitly pinned dependencies.
> 
> Add "make check" to run the python linters [in the host execution
> environment].
> 
> Add "make venv-check" which combines the above two: create/update the
> venv, then run the linters in that explicitly managed environment.
> 
> Add "make develop" which canonizes the runes needed to get both the
> linting pre-requisites (the "[devel]" part), and the editable
> live-install (the "-e" part) of these python libraries.
> 
> make clean: delete miscellaneous python packaging output possibly
> created by pipenv, pip, or other python packaging utilities
> 
> make distclean: delete the above, the .venv, and the editable "qemu"
> package forwarder (qemu.egg-info) if there is one.
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst |  3 +++
>  python/Makefile   | 42 ++
>  2 files changed, 45 insertions(+)
>  create mode 100644 python/Makefile
> 
> diff --git a/python/README.rst b/python/README.rst
> index e107bd12a69..3e09d20c23c 100644
> --- a/python/README.rst
> +++ b/python/README.rst
> @@ -35,6 +35,9 @@ Files in this directory
>  - ``qemu/`` Python package source directory.
>  - ``tests/`` Python package tests directory.
>  - ``avocado.cfg`` Configuration for the Avocado test-runner.
> +  Used by ``make check`` et al.
> +- ``Makefile`` provides some common testing/installation invocations.
> +  Try ``make help`` to see available targets.
>  - ``MANIFEST.in`` is read by python setuptools, it specifies additional files
>that should be included by a source distribution.
>  - ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
> diff --git a/python/Makefile b/python/Makefile
> new file mode 100644
> index 000..184f59e5634
> --- /dev/null
> +++ b/python/Makefile
> @@ -0,0 +1,42 @@
> +.PHONY: help venv venv-check check clean distclean develop
> +
> +help:
> + @echo "python packaging help:"
> + @echo ""
> + @echo "make venv:   Create pipenv's virtual environment."
> + @echo "NOTE: Requires Python 3.6 and pipenv."
> + @echo "  Will download packages from PyPI."
> + @echo "Hint: (On Fedora): 'sudo dnf install python36 pipenv'"
> + @echo ""
> + @echo "make venv-check: run linters using pipenv's virtual environment."
> + @echo "Hint: If you don't know which test to run, run this one!"
> + @echo ""
> + @echo "make develop:Install deps for 'make check', and"
> + @echo " the qemu libs in editable/development mode."
> + @echo ""
> + @echo "make check:  run linters using the current environment."
> + @echo ""

Let's observe how this will be used (or misused).  I fear most people
will jump into `make check`, even though you have described `make
venv-check` as the primary choice.

We have a precedent with `make check-acceptance` that will create a
venv and use it by default, so we can consider that as a fallback
strategy based on user feedback.

> + @echo "make clean:  remove build output."
> + @echo ""
> + @echo "make distclean:  remove venv files, qemu package forwarder, and"
> + @echo " everything from 'make clean'."
> +
> +venv: .venv
> +.venv: Pipfile.lock
> +     @PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
> + @touch .venv
> +
> +venv-check: venv
> + @pipenv run make check
> +
> +develop:
> + pip3 install -e .[devel]
> +
> +check:
> + @avocado --config avocado.cfg run tests/
> +
> +clean:
> + rm -rf build/ dist/
> +

Usually `python3 setup.py clean --all` would be the better choice here,
but, it doesn't clean `dist/`, so I'm OK with this.

> +distclean: clean
> + rm -rf qemu.egg-info/ .venv/
> -- 
> 2.30.2
> 
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 21/25] python: add avocado-framework and tests

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:37PM -0400, John Snow wrote:
> Try using avocado to manage our various tests; even though right now
> they're only invoking shell scripts and not really running any
> python-native code.
> 
> Create tests/, and add shell scripts which call out to mypy, flake8,
> pylint and isort to enforce the standards in this directory.
> 
> Add avocado-framework to the setup.cfg development dependencies, and add
> avocado.cfg to store some preferences for how we'd like the test output
> to look.
> 
> Finally, add avocado-framework to the Pipfile environment and lock the
> new dependencies. We are using avocado >= 87.0 here to take advantage of
> some features that Cleber has helpfully added to make the test output
> here *very* friendly and easy to read for developers that might chance
> upon the output in Gitlab CI.
> 
> [Note: ALL of the dependencies get updated to the most modern versions
> that exist at the time of this writing. No way around it that I have
> seen. Not ideal, but so it goes.]
> 
> Provided you have the right development dependencies (mypy, flake8,
> isort, pylint, and now avocado-framework) You should be able to run
> "avocado --config avocado.cfg run tests/" from the python folder to run
> all of these linters with the correct arguments.
> 
> (A forthcoming commit adds the much easier 'make check'.)
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst  |   2 +
>  python/Pipfile.lock| 104 ++---
>  python/avocado.cfg |  10 
>  python/setup.cfg   |   1 +
>  python/tests/flake8.sh |   2 +
>  python/tests/isort.sh  |   2 +
>  python/tests/mypy.sh   |   2 +
>  python/tests/pylint.sh |   2 +
>  8 files changed, 77 insertions(+), 48 deletions(-)
>  create mode 100644 python/avocado.cfg
>  create mode 100755 python/tests/flake8.sh
>  create mode 100755 python/tests/isort.sh
>  create mode 100755 python/tests/mypy.sh
>  create mode 100755 python/tests/pylint.sh
> 

With the patches from your "Python: delint python library" series:

   $ pipenv run avocado --config avocado.cfg run tests/
   JOB ID : b27b48eded8b405c6672e61e3d1561407fca9d5e
   JOB LOG: 
/home/cleber/avocado/job-results/job-2021-05-25T14.56-b27b48e/job.log
(1/4) tests/flake8.sh: PASS (0.67 s)
(2/4) tests/isort.sh: PASS (0.37 s)
(3/4) tests/mypy.sh: PASS (0.39 s)
(4/4) tests/pylint.sh: PASS (4.85 s)
   RESULTS: PASS 4 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
   JOB TIME   : 6.75 s

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 15/25] python: move mypy.ini into setup.cfg

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:31PM -0400, John Snow wrote:
> mypy supports reading its configuration values from a central project
> configuration file; do so.
> 
> Signed-off-by: John Snow 
> ---
>  python/mypy.ini  | 4 
>  python/setup.cfg | 5 +
>  2 files changed, 5 insertions(+), 4 deletions(-)
>  delete mode 100644 python/mypy.ini

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 13/25] python: add excluded dirs to flake8 config

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:29PM -0400, John Snow wrote:
> Instruct flake8 to avoid certain well-known directories created by
> python tooling that it ought not check.
> 
> Note that at-present, nothing actually creates a ".venv" directory; but
> it is in such widespread usage as a de-facto location for a developer's
> virtual environment that it should be excluded anyway. A forthcoming
> commit canonizes this with a "make venv" command.
> 
> Signed-off-by: John Snow 
> ---
>  python/setup.cfg | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 9ecb2902006..f21a1c42fc0 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -21,6 +21,8 @@ packages = find_namespace:
>  
>  [flake8]
>  extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
> +exclude = __pycache__,
> +  .venv,
>

Given that the default set of exclusions (version control system
files) are not expected here, it LGTM to reset it with these.

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 20/25] python: add devel package requirements to setuptools

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:36PM -0400, John Snow wrote:
> setuptools doesn't have a formal understanding of development requires,
> but it has an optional feataures section. Fine; add a "devel" feature
> and add the requirements to it.
> 
> To avoid duplication, we can modify pipenv to install qemu[devel]
> instead. This enables us to run invocations like "pip install -e
> .[devel]" and test the package on bleeding-edge packages beyond those
> specified in Pipfile.lock.
> 
> Importantly, this also allows us to install the qemu development
> packages in a non-networked mode: `pip3 install --no-index -e .[devel]`
> will now fail if the proper development dependencies are not already
> met. This can be useful for automated build scripts where fetching
> network packages may be undesirable.
>

This is a fairly exotic feature of setuptools, with very very few
packages that I know about using it.  With most users (I believe)
relying on pipenv to get the exact packages, the setuptools/pip use
case may fall into obscurity IMO.

So my suggestion is: consider better exposing the fact that this is
available (a documentation section perhaps).

> Signed-off-by: John Snow 
> ---
>  python/Pipfile  |  5 +
>  python/Pipfile.lock | 14 +-
>  python/setup.cfg|  9 +++++
>  3 files changed, 19 insertions(+), 9 deletions(-)
>

Either way,

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 18/25] python/qemu: add isort to pipenv

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:34PM -0400, John Snow wrote:
> isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
> certain "from ..." clauses that are not related to imports.
> 
> isort < 5.1.1 has a bug where it does not handle comments near import
> statements correctly.
> 
> Require 5.1.2 or greater.
> 
> isort can be run with 'isort -c qemu' from the python root.
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile  | 1 +
>  python/Pipfile.lock | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 11/25] python: add pylint to pipenv

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:27PM -0400, John Snow wrote:
> We are specifying >= pylint 2.7.x for several reasons:
> 
> 1. For setup.cfg support, added in pylint 2.5.x
> 2. To specify a version that has incompatibly dropped
>bad-whitespace checks (2.6.x)
> 3. 2.7.x fixes "unsubscriptable" warnings in Python 3.9
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile  |   1 +
>  python/Pipfile.lock | 130 
>  2 files changed, 131 insertions(+)
>  create mode 100644 python/Pipfile.lock
> 

Works as expected:

   $ pipenv run pylint --version 
   pylint 2.7.2
   astroid 2.5.2
   Python 3.6.13 (default, Feb 25 2021, 00:00:00) 
   [GCC 11.0.0 20210210 (Red Hat 11.0.0-0)]

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 07/25] python: add MANIFEST.in

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:23PM -0400, John Snow wrote:
> When creating a source distribution via 'python3 setup.py sdist', the
> VERSION and PACKAGE.rst files aren't bundled by default. Create a
> MANIFEST.in file that instructs the build tools to include these so that
> installation from source dists won't fail.
> 
> (This invocation is required by 'tox', as well as by the tooling needed
> to upload packages to PyPI.)
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst  | 2 ++
>  python/MANIFEST.in | 2 ++
>  2 files changed, 4 insertions(+)
>  create mode 100644 python/MANIFEST.in
>

I was about to propose mypy.ini to be included here, but given
that it's merged into setup.cfg later in this series:

Reviewed-by: Cleber Rosa 

---

Note to self (and to you) when generating the sdist, I get:

   ...
   package init file 'qemu/__init__.py' not found (or not a regular file)
   package init file 'dist/__init__.py' not found (or not a regular file)
   ...

Which may not be too harmful, but deserves investigation.


signature.asc
Description: PGP signature

Re: [PATCH v6 06/25] python: add directory structure README.rst files

[Cleber Rosa]

ex 000..c21951491cf
> --- /dev/null
> +++ b/python/qemu/qmp/README.rst
> @@ -0,0 +1,9 @@
> +qemu.qmp package
> +
> +
> +This package provides a library used for connecting to and communicating
> +with QMP servers. It is used extensively by iotests, vm tests,
> +acceptance tests, and other utilities in the ./scripts directory. It is
> +not a fully-fledged SDK and is subject to change at any time.
> +
> +See the documentation in ``__init__.py`` for more information.
> diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
> new file mode 100644
> index 000..975fbf4d7de
> --- /dev/null
> +++ b/python/qemu/utils/README.rst
> @@ -0,0 +1,7 @@
> +qemu.utils package
> +======
> +
> +This package provides miscellaneous utilities used for testing and
> +debugging QEMU. It is used primarily by the vm and acceptance tests.
> +
> +See the documentation in ``__init__.py`` for more information.
> -- 
> 2.30.2
> 
> 

With the ``setup.py`` and ``setuptools`` for consistency sake
mentioned in my first comment, all other comments are suggestions, so:

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 04/25] python: add qemu package installer

[Cleber Rosa]

n/setup.py b/python/setup.py
> new file mode 100755
> index 000..2014f81b757
> --- /dev/null
> +++ b/python/setup.py
> @@ -0,0 +1,23 @@
> +#!/usr/bin/env python3
> +"""
> +QEMU tooling installer script
> +Copyright (c) 2020-2021 John Snow for Red Hat, Inc.
> +"""
> +
> +import setuptools
> +import pkg_resources
> +
> +
> +def main():
> +"""
> +QEMU tooling installer
> +"""
> +
> +# 
> https://medium.com/@daveshawley/safely-using-setup-cfg-for-metadata-1babbe54c108
> +pkg_resources.require('setuptools>=39.2')
> +
> +setuptools.setup()
> +
> +
> +if __name__ == '__main__':
> +main()
> -- 
> 2.30.2
> 
> 

BTW, about the need to have a "setup.py", before pip 21.1.1:

  $ rm setup.py
  $ pip install -e .
  ERROR: File "setup.py" not found. Directory cannot be installed in editable 
mode: /home/cleber/src/qemu/python

On pip 21.1.1:

   $ pip install -e . 
   Obtaining file:///home/cleber/src/qemu/python
   Installing collected packages: qemu
 Running setup.py develop for qemu
   Successfully installed qemu-0.0.0

Side note: The "Running setup.py ..." message given by pip 21.1.1,
even though there is *not* a "setup.py" is rather confusing.

Anyway, we may be able to drop setup.py either when we find pip 21.1.1
or later in our "common build environments", or if we require people
hacking on the Python module to "pip install --upgrade pip".

I'll be repeating myself here, but I believe you made the right
choices at this time, and based on my testing I can successfully
install/develop using "python setup.py" and "pip", so:

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 03/25] python: create utils sub-package

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:19PM -0400, John Snow wrote:
> Create a space for miscellaneous things that don't belong strictly in
> "qemu.machine" nor "qemu.qmp" packages.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/__init__.py   |  8 
>  python/qemu/utils/__init__.py | 23 +++
>  python/qemu/{machine => utils}/accel.py   |  0
>  tests/acceptance/avocado_qemu/__init__.py |  4 ++--
>  tests/acceptance/virtio-gpu.py|  2 +-
>  tests/vm/aarch64vm.py |  2 +-
>  tests/vm/basevm.py|  3 ++-
>  7 files changed, 29 insertions(+), 13 deletions(-)
>  create mode 100644 python/qemu/utils/__init__.py
>  rename python/qemu/{machine => utils}/accel.py (100%)
> 

As you mentioned in the previous patch notes, I would not mind a
squash here.  Either way:

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v6 02/25] python: create qemu packages

[Cleber Rosa]

On Wed, May 12, 2021 at 07:12:18PM -0400, John Snow wrote:
> move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
> directives across the tree.
> 
> This is done to create a PEP420 namespace package, in which we may
> create subpackages. To do this, the namespace directory ("qemu") should
> not have any modules in it. Those files will go into new 'machine' and 'qmp'
> subpackages instead.
> 
> Implement machine/__init__.py making the top-level classes and functions
> from its various modules available directly inside the package. Change
> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> library classes are available directly from "qemu.qmp" instead of
> "qemu.qmp.qmp".
> 
> Signed-off-by: John Snow 
> 
> 
> ---
> 
> Note for reviewers: in the next patch, I add a utils sub-package and
> move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
> way, we can squash it in here if we want, or just leave it as its own
> follow-up patch, I am just leaving it as something that will be easy to
> un-do or change for now.
> 
> Signed-off-by: John Snow 
> ---
>  python/{qemu => }/.isort.cfg|  0
>  python/qemu/__init__.py | 11 --
>  python/qemu/{ => machine}/.flake8   |  0
>  python/qemu/machine/__init__.py | 41 +
>  python/qemu/{ => machine}/accel.py  |  0
>  python/qemu/{ => machine}/console_socket.py |  0
>  python/qemu/{ => machine}/machine.py| 16 +---
>  python/qemu/{ => machine}/pylintrc  |  0
>  python/qemu/{ => machine}/qtest.py  |  3 +-
>  python/qemu/{qmp.py => qmp/__init__.py} | 12 +-
>  tests/acceptance/avocado_qemu/__init__.py   |  4 +-
>  tests/acceptance/virtio-gpu.py  |  2 +-
>  tests/qemu-iotests/300  |  4 +-
>  tests/qemu-iotests/iotests.py   |  2 +-
>  tests/vm/aarch64vm.py   |  2 +-
>  tests/vm/basevm.py  |  3 +-
>  16 files changed, 73 insertions(+), 27 deletions(-)
>  rename python/{qemu => }/.isort.cfg (100%)
>  delete mode 100644 python/qemu/__init__.py
>  rename python/qemu/{ => machine}/.flake8 (100%)
>  create mode 100644 python/qemu/machine/__init__.py
>  rename python/qemu/{ => machine}/accel.py (100%)
>  rename python/qemu/{ => machine}/console_socket.py (100%)
>  rename python/qemu/{ => machine}/machine.py (98%)
>  rename python/qemu/{ => machine}/pylintrc (100%)
>  rename python/qemu/{ => machine}/qtest.py (99%)
>  rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
>

Hi John,

Thanks for this!  I went through it and it LGTM.  I've tested it with
iotests, acceptance and with a vm-build-fedora.

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

On Wed, Feb 17, 2021 at 12:28:22PM -0500, John Snow wrote:
> On 2/16/21 10:02 PM, Cleber Rosa wrote:
> > On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
> > > On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> > > > pipenv is a tool used for managing virtual environments with pinned,
> > > > explicit dependencies. It is used for precisely recreating python
> > > > virtual environments.
> > > > 
> > > > pipenv uses two files to do this:
> > > > 
> > > > (1) Pipfile, which is similar in purpose and scope to what setup.py
> > > > lists. It specifies the requisite minimum to get a functional
> > > > environment for using this package.
> > > > 
> > > > (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> > > > requirements.txt`. It specifies a canonical virtual environment used for
> > > > deployment or testing. This ensures that all users have repeatable
> > > > results.
> > > > 
> > > > The primary benefit of using this tool is to ensure repeatable CI
> > > > results with a known set of packages. Although I endeavor to support as
> > > > many versions as I can, the fluid nature of the Python toolchain often
> > > > means tailoring code for fairly specific versions.
> > > > 
> > > > Note that pipenv is *not* required to install or use this module; this 
> > > > is
> > > > purely for the sake of repeatable testing by CI or developers.
> > > > 
> > > > Here, a "blank" pipfile is added with no dependencies, but specifies
> > > > Python 3.6 for the virtual environment.
> > > > 
> > > > Pipfile will specify our version minimums, while Pipfile.lock specifies
> > > > an exact loudout of packages that were known to operate correctly. This
> > > 
> > > Layout? Loadout?
> > > 
> > > > latter file provides the real value for easy setup of container images
> > > > and CI environments.
> > > > 
> > > > Signed-off-by: John Snow 
> > > > ---
> > > >   python/Pipfile | 11 +++
> > > >   1 file changed, 11 insertions(+)
> > > >   create mode 100644 python/Pipfile
> > > > 
> > > 
> > > Other than that,
> > > 
> > > Reviewed-by: Cleber Rosa 
> > 
> > Actually, just one suggestion: bump the position of this patch twice.
> > It makes it easier to understand its purpose if it is placed right
> > before the "python: add pylint to pipenv" patch.
> > 
> > Cheers,
> > - Cleber.
> > 
> 
> The way the series is laid out is:
> 
> 01-02: pre-requisite fixes
> 03-07: Create the package, readmes, etc.
> 08:Pipenv support
> 09-11: Pylint
> 12-13: flake8
> 14-15: mypy
> 16-17: isort
> 18-20: Testing and pre-requisites
> 21-23: Polish
> 24: CI support
> 
> Moving the pipenv patch to just before the final pylint patch works OK, but
> breaks up the pylint section. Should I still do it?
>

OK, now with that approach of groupping in min, it sounds reasonable.
My previous point was that pipenv is not needed until right before #10,
but that's just nitpicking and almost bikeshedding (I won't admit it
easily).

> --js
> 
> 
> (Hm, by this layout, I should probably actually move the pylint fix in #01
> down to appear after the pipenv patch. I could also move the flake8 fixes in
> #21 up to be near the other flake8 patches.)

Sounds more consistent.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v4 18/24] python/qemu: add qemu package itself to pipenv

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:50PM -0500, John Snow wrote:
> This adds the python qemu packages themselves to the pipenv manifest.
> 'pipenv sync' will create a virtual environment sufficient to use the SDK.
> 'pipenv sync --dev' will create a virtual environment sufficient to use
> and test the SDK (with pylint, mypy, isort, flake8, etc.)
> 
> The qemu packages are installed in 'editable' mode; all changes made to
> the python package inside the git tree will be reflected in the
> installed package without reinstallation. This includes changes made
> via git pull and so on.
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile  | 1 +
>  python/Pipfile.lock | 9 +++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 17/24] python/qemu: add isort to pipenv

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:49PM -0500, John Snow wrote:
> isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
> certain "from ..." clauses that are not related to imports.
> 
> Require 5.0.5 or greater.
> 
> isort can be run with 'isort -c qemu' from the python root.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  python/Pipfile  | 1 +
>  python/Pipfile.lock | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 16/24] python: move .isort.cfg into setup.cfg

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:48PM -0500, John Snow wrote:
> Signed-off-by: John Snow 
> ---
>  python/.isort.cfg | 7 ---
>  python/setup.cfg  | 8 
>  2 files changed, 8 insertions(+), 7 deletions(-)
>  delete mode 100644 python/.isort.cfg
> 

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 15/24] python: add mypy to pipenv

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:47PM -0500, John Snow wrote:
> 0.730 appears to be about the oldest version that works with the
> features we want, including nice human readable output (to make sure
> iotest 297 passes), and type-parameterized Popen generics.
> 
> 0.770, however, supports adding 'strict' to the config file, so require
> at least 0.770.
> 
> Now that we are checking a namespace package, we need to tell mypy to
> allow PEP420 namespaces, so modify the mypy config as part of the move.
> 
> mypy can now be run from the python root by typing 'mypy qemu'.
>

 $ mypy qemu 
 qemu/utils/accel.py: error: Source file found twice under different module 
names: 'qmp' and 'qemu.qmp'
 Found 1 error in 1 file (errors prevented further checking)

I guess you meant 'mypy -p qemu'.

> Signed-off-by: John Snow 
> ---
>  python/Pipfile  |  1 +
>  python/Pipfile.lock | 37 -
>  python/setup.cfg|  1 +
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 

With that change,

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 13/24] python: Add flake8 to pipenv

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:45PM -0500, John Snow wrote:
> flake8 3.5.x does not support the --extend-ignore syntax used in the
> .flake8 file to gracefully extend default ignores, so 3.6.x is our
> minimum requirement. There is no known upper bound.
> 
> flake8 can be run from the python/ directory with no arguments.
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile  |  1 +
>  python/Pipfile.lock | 51 -
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 12/24] python: move flake8 config to setup.cfg

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:44PM -0500, John Snow wrote:
> Update the comment concerning the flake8 exception to match commit
> 42c0dd12, whose commit message stated:
> 
> A note on the flake8 exception: flake8 will warn on *any* bare except,
> but pylint's is context-aware and will suppress the warning if you
> re-raise the exception.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/.flake8 | 2 --
>  python/setup.cfg| 3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
>  delete mode 100644 python/qemu/machine/.flake8
>

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 11/24] python: add pylint to pipenv

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:43PM -0500, John Snow wrote:
> We are specifying >= pylint 2.6.x for two reasons:
> 
> 1. For setup.cfg support, added in pylint 2.5.x
> 2. To clarify that we are using a version that has incompatibly dropped
> bad-whitespace checks.
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile  |   1 +
>  python/Pipfile.lock | 137 
>  2 files changed, 138 insertions(+)
>  create mode 100644 python/Pipfile.lock
> 

Tested at this point with:

 $ pipenv install --dev
 $ pipenv run pip freeze
 astroid==2.4.2
 isort==5.7.0
 lazy-object-proxy==1.4.3
 mccabe==0.6.1
 pylint==2.6.0
 six==1.15.0
 toml==0.10.2
 typed-ast==1.4.2
 wrapt==1.12.1

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 00/24] python: create installable package

[Cleber Rosa]

On Mon, Feb 15, 2021 at 04:32:29PM -0500, John Snow wrote:
> On 2/11/21 9:52 PM, Cleber Rosa wrote:
> > On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
> > > This series factors the python/qemu directory as an installable
> > > package. It does not yet actually change the mechanics of how any other
> > > python source in the tree actually consumes it (yet), beyond the import
> > > path. (some import statements change in a few places.)
> > > 
> > > git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
> > > CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
> > > (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
> > > 
> > > The primary motivation of this series is primarily to formalize our
> > > dependencies on mypy, flake8, isort, and pylint alongside versions that
> > > are known to work. It does this using the setup.cfg and setup.py
> > > files. It also adds explicitly pinned versions (using Pipfile.lock) of
> > > these dependencies that should behave in a repeatable and known way for
> > > developers and CI environments both. Lastly, it enables those CI checks
> > > such that we can enforce Python coding quality checks via the CI tests.
> > > 
> > > An auxiliary motivation is that this package is formatted in such a way
> > > that it COULD be uploaded to https://pypi.org/project/qemu and installed
> > > independently of qemu.git with `pip install qemu`, but that button
> > > remains *unpushed* and this series *will not* cause any such
> > > releases. We have time to debate finer points like API guarantees and
> > > versioning even after this series is merged.
> > > 
> > > Some other things this enables that might be of interest:
> > > 
> > > With the python tooling as a proper package, you can install this
> > > package in editable or production mode to a virtual environment, your
> > > local user environment, or your system packages. The primary benefit of
> > > this is to gain access to QMP tooling regardless of CWD, without needing
> > > to battle sys.path (and confounding other python analysis tools).
> > > 
> > > For example: when developing, you may go to qemu/python/ and run `make
> > > venv` followed by `pipenv shell` to activate a virtual environment that
> > > contains the qemu python packages. These packages will always reflect
> > > the current version of the source files in the tree. When you are
> > > finished, you can simply exit the shell (^d) to remove these packages
> > > from your python environment.
> > > 
> > > When not developing, you could install a version of this package to your
> > > environment outright to gain access to the QMP and QEMUMachine classes
> > > for lightweight scripting and testing by using pip: "pip install [--user] 
> > > ."
> > > 
> > > TESTING THIS SERIES:
> > > 
> > > First of all, nothing should change. Without any intervention,
> > > everything should behave exactly as it was before. The only new
> > > information here comes from how to interact with and run the linters
> > > that will be enforcing code quality standards in this subdirectory.
> > > 
> > > To test those, CD to qemu/python first, and then:
> > > 
> > > 1. Try "make venv && pipenv shell" to get a venv with the package
> > > installed to it in editable mode. Ctrl+d exits this venv shell. While in
> > > this shell, any python script that uses "from qemu.[qmp|machine] import
> > > ..." should work correctly regardless of where the script is, or what
> > > your CWD is.
> > > 
> > 
> > Ack here, works as expected.
> > 
> 
> That's a relief!
> 
> > > You will need Python 3.6 installed on your system to do this step. For
> > > Fedora: "dnf install python3.6" will do the trick.
> > > 
> > 
> > You may have explained this before, so forgive me asking again.  Why
> > is this dependent on a given Python version, and not a *minimum*
> > Python version? Are the checkers themselves susceptible to different
> > behavior depending on the Python version used?  If so, any hint on the
> > strategy for developing with say, Python 3.8, and then having issues
> > caught only on Python 3.6?
> > 
> 
> It's a good question, and I have struggled with communicating the language.
> So here are a few points:
> 
> (1) Users will not need Python 3.6 on their local systems t

Re: [PATCH v4 10/24] python: move pylintrc into setup.cfg

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:42PM -0500, John Snow wrote:
> Delete the empty settings now that it's sharing a home with settings for
> other tools.
> 
> pylint can now be run from this folder as "pylint qemu".
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/pylintrc | 58 
>  python/setup.cfg | 29 ++
>  2 files changed, 29 insertions(+), 58 deletions(-)
>  delete mode 100644 python/qemu/machine/pylintrc
>

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 09/24] python: add pylint import exceptions

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:41PM -0500, John Snow wrote:
> Pylint 2.5.x and 2.6.x have regressions that make import checking
> inconsistent, see:
> 
> https://github.com/PyCQA/pylint/issues/3609
> https://github.com/PyCQA/pylint/issues/3624
> https://github.com/PyCQA/pylint/issues/3651
> 
> Pinning to 2.4.4 is worse, because it mandates versions of shared
> dependencies that are too old for features we want in isort and mypy.
> Oh well.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/__init__.py | 3 +++
>  python/qemu/machine/machine.py  | 2 +-
>  python/qemu/machine/qtest.py| 2 +-
>  3 files changed, 5 insertions(+), 2 deletions(-)
>

I can see your struggle on those issues, so I choose not to punish
myself attempting to replicate them.  I'll forever trust you in these
matters.

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> > pipenv is a tool used for managing virtual environments with pinned,
> > explicit dependencies. It is used for precisely recreating python
> > virtual environments.
> > 
> > pipenv uses two files to do this:
> > 
> > (1) Pipfile, which is similar in purpose and scope to what setup.py
> > lists. It specifies the requisite minimum to get a functional
> > environment for using this package.
> > 
> > (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> > requirements.txt`. It specifies a canonical virtual environment used for
> > deployment or testing. This ensures that all users have repeatable
> > results.
> > 
> > The primary benefit of using this tool is to ensure repeatable CI
> > results with a known set of packages. Although I endeavor to support as
> > many versions as I can, the fluid nature of the Python toolchain often
> > means tailoring code for fairly specific versions.
> > 
> > Note that pipenv is *not* required to install or use this module; this is
> > purely for the sake of repeatable testing by CI or developers.
> > 
> > Here, a "blank" pipfile is added with no dependencies, but specifies
> > Python 3.6 for the virtual environment.
> > 
> > Pipfile will specify our version minimums, while Pipfile.lock specifies
> > an exact loudout of packages that were known to operate correctly. This
> 
> Layout? Loadout?
> 
> > latter file provides the real value for easy setup of container images
> > and CI environments.
> > 
> > Signed-off-by: John Snow 
> > ---
> >  python/Pipfile | 11 +++
> >  1 file changed, 11 insertions(+)
> >  create mode 100644 python/Pipfile
> >
> 
> Other than that,
> 
> Reviewed-by: Cleber Rosa 

Actually, just one suggestion: bump the position of this patch twice.
It makes it easier to understand its purpose if it is placed right
before the "python: add pylint to pipenv" patch.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> pipenv is a tool used for managing virtual environments with pinned,
> explicit dependencies. It is used for precisely recreating python
> virtual environments.
> 
> pipenv uses two files to do this:
> 
> (1) Pipfile, which is similar in purpose and scope to what setup.py
> lists. It specifies the requisite minimum to get a functional
> environment for using this package.
> 
> (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> requirements.txt`. It specifies a canonical virtual environment used for
> deployment or testing. This ensures that all users have repeatable
> results.
> 
> The primary benefit of using this tool is to ensure repeatable CI
> results with a known set of packages. Although I endeavor to support as
> many versions as I can, the fluid nature of the Python toolchain often
> means tailoring code for fairly specific versions.
> 
> Note that pipenv is *not* required to install or use this module; this is
> purely for the sake of repeatable testing by CI or developers.
> 
> Here, a "blank" pipfile is added with no dependencies, but specifies
> Python 3.6 for the virtual environment.
> 
> Pipfile will specify our version minimums, while Pipfile.lock specifies
> an exact loudout of packages that were known to operate correctly. This

Layout? Loadout?

> latter file provides the real value for easy setup of container images
> and CI environments.
> 
> Signed-off-by: John Snow 
> ---
>  python/Pipfile | 11 +++
>  1 file changed, 11 insertions(+)
>  create mode 100644 python/Pipfile
>

Other than that,

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 07/24] python: add directory structure README.rst files

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:39PM -0500, John Snow wrote:
> Add short readmes to python/, python/qemu/, python/qemu/machine,
> python/qemu/qmp, and python/qemu/utils that explain the directory
> hierarchy. These readmes are visible when browsing the source on
> e.g. gitlab/github and are designed to help new developers/users quickly
> make sense of the source tree.
> 
> They are not designed for inclusion in a published manual.
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst  | 41 ++
>  python/qemu/README.rst |  8 +++
>  python/qemu/machine/README.rst |  9 
>  python/qemu/qmp/README.rst |  9 
>  python/qemu/utils/README.rst   |  9 
>  5 files changed, 76 insertions(+)
>  create mode 100644 python/README.rst
>  create mode 100644 python/qemu/README.rst
>  create mode 100644 python/qemu/machine/README.rst
>  create mode 100644 python/qemu/qmp/README.rst
>  create mode 100644 python/qemu/utils/README.rst
>

It's not often I complain about too much documentation, but I honestly
think this will not scale.  I understand that the intention is to help
users browsing through the directory structure it has a huge potential
for bit rot.

The READMEs at the first two levels seem OK, but the ones at the
subpackages level will be a maintainance nightmare.  I would *very
much* move those (subpackage ones) documentation into the Python file
themselves.

> diff --git a/python/README.rst b/python/README.rst
> new file mode 100644
> index 000..6a14b99e104
> --- /dev/null
> +++ b/python/README.rst
> @@ -0,0 +1,41 @@
> +QEMU Python Tooling
> +===
> +
> +This directory houses Python tooling used by the QEMU project to build,
> +configure, and test QEMU. It is organized by namespace (``qemu``), and
> +then by package (``qemu/machine``, ``qemu/qmp``).
> +
> +``setup.py`` is used by ``pip`` to install this tooling to the current
> +environment. ``setup.cfg`` provides the packaging configuration used by
> +setup.py in a setuptools specific format. You will generally invoke it
> +by doing one of the following:
> +
> +1. ``pip3 install .`` will install these packages to your current
> +   environment. If you are inside a virtual environment, they will
> +   install there. If you are not, it will attempt to install to the
> +   global environment, which is not recommended.
> +
> +2. ``pip3 install --user .`` will install these packages to your user's
> +   local python packages. If you are inside of a virtual environment,
> +   this will fail.
> +
> +If you amend the ``-e`` argument, pip will install in "editable" mode;
> +which installs a version of the package that installs a forwarder
> +pointing to these files, such that the package always reflects the
> +latest version in your git tree.
> +
> +See `Installing packages using pip and virtual environments
> +`_
> +for more information.
> +
> +
> +Files in this directory
> +---
> +
> +- ``qemu/`` Python package source directory.
> +- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
> +- ``README.rst`` you are here!
> +- ``VERSION`` contains the PEP-440 compliant version used to describe
> +  this package; it is referenced by ``setup.cfg``.
> +- ``setup.cfg`` houses setuptools package configuration.
> +- ``setup.py`` is the setuptools installer used by pip; See above.
> diff --git a/python/qemu/README.rst b/python/qemu/README.rst
> new file mode 100644
> index 000..d04943f526c
> --- /dev/null
> +++ b/python/qemu/README.rst
> @@ -0,0 +1,8 @@
> +QEMU Python Namespace
> +=
> +
> +This directory serves as the root of a `Python PEP 420 implicit
> +namespace package `_.
> +
> +Each directory below is assumed to be an installable Python package that
> +is available under the ``qemu.`` namespace.
> diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
> new file mode 100644
> index 000..ac2b4fffb42
> --- /dev/null
> +++ b/python/qemu/machine/README.rst
> @@ -0,0 +1,9 @@
> +qemu.machine package
> +
> +
> +This package provides core utilities used for testing and debugging
> +QEMU. It is used by the iotests, vm tests, acceptance tests, and several
> +other utilities in the ./scripts directory. It is not a fully-fledged
> +SDK and it is subject to change at any time.
> +
> +See the documentation in ``__init__.py`` for more information.
> diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
> new file mode 100644
> index 000..c21951491cf
> --- /dev/null
> +++ b/python/qemu/qmp/README.rst
> @@ -0,0 +1,9 @@
> +qemu.qmp package
> +
> +
> +This package provides a library used for connecting to and communicating
> +with QMP servers. It is used extensively by iotests, vm tests,
> +acceptance tests, and

Re: [PATCH v4 06/24] python: add VERSION file

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:38PM -0500, John Snow wrote:
> Python infrastructure as it exists today is not capable reliably of
> single-sourcing a package version from a parent directory. The authors
> of pip are working to correct this, but as of today this is not possible.
> 
> The problem is that when using pip to build and install a python
> package, it copies files over to a temporary directory and performs its
> build there. This loses access to any information in the parent
> directory, including git itself.
> 
> Further, Python versions have a standard (PEP 440) that may or may not
> follow QEMU's versioning. In general, it does; but naturally QEMU does
> not follow PEP 440. To avoid any automatically-generated conflict, a
> manual version file is preferred.
> 
> 
> I am proposing:
> 
> - Python tooling follows the QEMU version, indirectly, but with a major
>   version of 0 to indicate that the API is not expected to be
>   stable. This would mean version 0.5.2.0, 0.5.1.1, 0.5.3.0, etc.
> 
> - In the event that a Python package needs to be updated independently
>   of the QEMU version, a pre-release alpha version should be preferred,
>   but *only* after inclusion to the qemu development or stable branches.
> 
>   e.g. 0.5.2.0a1, 0.5.2.0a2, and so on should be preferred prior to
>   5.2.0's release.
> 
> - The Python core tooling makes absolutely no version compatibility
>   checks or constraints. It *may* work with releases of QEMU from the
>   past or future, but it is not required to.
> 
>   i.e., "qemu.machine" will, for now, remain in lock-step with QEMU.
> 
> - We reserve the right to split the qemu package into independently
>   versioned subpackages at a later date. This might allow for us to
>   begin versioning QMP independently from QEMU at a later date, if
>   we so choose.
> 
> 
> Implement this versioning scheme by adding a VERSION file and setting it
> to 0.6.0.0a1.
> 
> Signed-off-by: John Snow 
> ---
>  python/VERSION   | 1 +
>  python/setup.cfg | 1 +
>  2 files changed, 2 insertions(+)
>  create mode 100644 python/VERSION
> 

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 05/24] python: add qemu package installer

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:37PM -0500, John Snow wrote:
> Add setup.cfg and setup.py, necessary for installing a package via
> pip. Add a rst document explaining the basics of what this package is

Nitpick 1: setup.cfg and setup.py are indeed used by pip, your
statement is correct.  But, it hides the fact that these can be used
without pip.  On a source tree based install, you may want to simply
use "python setup.py develop" to achieve what "pip install -e" would
do (without pip ever entering the picture).

Nitpick 2: while most people will understand what you mean by "rst
document", I believe that "Add a README file in reStructuredText
format" would be more obvious.

> for and who to contact for more information. This document will be used
> as the landing page for the package on PyPI.
> 
> I am not yet using a pyproject.toml style package manifest, because
> "editable" installs are not defined by PEP-517 and pip did not have
> support for this for some time; I consider the feature necessary for
> development.
>

I'm glad you kept it like this... I bet there's going to be another
PEP out, replacing the status quo, by the time I finish this review.

> Use a light-weight setup.py instead.
> 
> Signed-off-by: John Snow 
> ---
>  python/PACKAGE.rst | 32 
>  python/setup.cfg   | 19 +++
>  python/setup.py| 23 +++
>  3 files changed, 74 insertions(+)
>  create mode 100644 python/PACKAGE.rst
>  create mode 100644 python/setup.cfg
>  create mode 100755 python/setup.py
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> new file mode 100644
> index 000..0e714c87eb3
> --- /dev/null
> +++ b/python/PACKAGE.rst
> @@ -0,0 +1,32 @@
> +QEMU Python Tooling
> +===
> +
> +This package provides QEMU tooling used by the QEMU project to build,
> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
> +to change at any time.
> +
> +Usage
> +-
> +
> +The ``qemu.qmp`` subpackage provides a library for communicating with
> +QMP servers. The ``qemu.machine`` subpackage offers rudimentary
> +facilities for launching and managing QEMU processes. Refer to each
> +package's documentation
> +(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
> +for more information.
> +

This gives the impression that those are the only subpackages (and
they were).  Better to reword it taking into account the qemu.utils
subpackage and possibly others (leave it open so that it doesn't rot
so quickly).

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v4 04/24] python: create utils sub-package

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:36PM -0500, John Snow wrote:
> Create a space for miscellaneous things that don't belong strictly in
> "qemu.machine" nor "qemu.qmp" packages.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/__init__.py |  8 
>  python/qemu/utils/__init__.py   | 23 +++
>  python/qemu/{machine => utils}/accel.py |  0
>  tests/acceptance/boot_linux.py  |  2 +-
>  tests/acceptance/virtio-gpu.py  |  2 +-
>  tests/acceptance/virtiofs_submounts.py  |  2 +-
>  tests/vm/aarch64vm.py   |  2 +-
>  tests/vm/basevm.py  |  3 ++-
>  8 files changed, 29 insertions(+), 13 deletions(-)
>  create mode 100644 python/qemu/utils/__init__.py
>  rename python/qemu/{machine => utils}/accel.py (100%)
> 
> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
> index 27b0b19abd3..891a8f784b5 100644
> --- a/python/qemu/machine/__init__.py
> +++ b/python/qemu/machine/__init__.py
> @@ -8,10 +8,6 @@
>   - QEMUQtestMachine: VM class, with a qtest socket.
>  
>  - QEMUQtestProtocol: Connect to, send/receive qtest messages.
> -
> -- list_accel: List available accelerators
> -- kvm_available: Probe for KVM support
> -- tcg_available: Probe for TCG support
>  """
>  
>  # Copyright (C) 2020 John Snow for Red Hat Inc.
> @@ -26,15 +22,11 @@
>  # the COPYING file in the top-level directory.
>  #
>  
> -from .accel import kvm_available, list_accel, tcg_available
>  from .machine import QEMUMachine
>  from .qtest import QEMUQtestMachine, QEMUQtestProtocol
>  
>  
>  __all__ = (
> -'list_accel',
> -'kvm_available',
> -'tcg_available',
>  'QEMUMachine',
>  'QEMUQtestProtocol',
>  'QEMUQtestMachine',
> diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
> new file mode 100644
> index 000..edf807a93e5
> --- /dev/null
> +++ b/python/qemu/utils/__init__.py
> @@ -0,0 +1,23 @@
> +"""
> +QEMU development and testing utilities
> +
> +This library provides a small handful of utilities for performing various 
> tasks
> +not directly related to the launching of a VM.
> +
> +The only module included at present is accel; its public functions are
> +repeated here for your convenience:
> +
> +- list_accel: List available accelerators
> +- kvm_available: Probe for KVM support
> +- tcg_available: Prove for TCG support
> +"""
> +
> +# pylint: disable=import-error
> +from .accel import kvm_available, list_accel, tcg_available
> +
> +
> +__all__ = (
> +'list_accel',
> +'kvm_available',
> +'tcg_available',
> +)
> diff --git a/python/qemu/machine/accel.py b/python/qemu/utils/accel.py
> similarity index 100%
> rename from python/qemu/machine/accel.py
> rename to python/qemu/utils/accel.py
> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
> index 212365fd185..824cf03d5f4 100644
> --- a/tests/acceptance/boot_linux.py
> +++ b/tests/acceptance/boot_linux.py
> @@ -12,7 +12,7 @@
>  
>  from avocado_qemu import Test, BUILD_DIR
>  
> -from qemu.machine import kvm_available, tcg_available
> +from qemu.utils import kvm_available, tcg_available
>  

With the latest changes merged earlier Today, this won't be necessary
anymore on boot_linux.py, but the equivalent change will be necessary
on tests/acceptance/avocado_qemu/__init__.py.

With the change mentioned above (which you would catch on a rebase):

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

On Mon, Feb 15, 2021 at 05:04:24PM -0500, John Snow wrote:
> On 2/11/21 5:01 PM, Cleber Rosa wrote:
> > Closing a file that is open for writing, and then reading from it
> > sounds like a better idea than the opposite, given that the content
> > will be flushed.
> > 
> > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> > Signed-off-by: Cleber Rosa 
> > ---
> >   python/qemu/machine.py | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 7a40f4604b..6e44bda337 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -337,12 +337,12 @@ class QEMUMachine:
> 
> Is there a way to improve context for python functions? What method is this
> in? etc.
> 
> >   self._qmp.close()
> >   self._qmp_connection = None
> > -self._load_io_log()
> > -
> >   if self._qemu_log_file is not None:
> >   self._qemu_log_file.close()
> >   self._qemu_log_file = None
> > +self._load_io_log()
> > +
> >   self._qemu_log_path = None
> >   if self._temp_dir is not None:
> > 
> 
> Yeh, seems fine, though as wainer points out the interdependencies between
> _load_io_log, _qemu_log_file and _qemu_log_path are not all strictly clear,
> so it seems fragile.
>

Yep, agreed.  This was a first, conservative change.  Expect more later.

> But, this is more correct than it was, so:
> 
> Reviewed-by: John Snow 

Thanks,
- Cleber


signature.asc
Description: PGP signature

Re: [PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote:
> Hi,
> 
> On 2/11/21 7:01 PM, Cleber Rosa wrote:
> > Closing a file that is open for writing, and then reading from it
> > sounds like a better idea than the opposite, given that the content
> > will be flushed.
> > 
> > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
> > Signed-off-by: Cleber Rosa 
> > ---
> >   python/qemu/machine.py | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 7a40f4604b..6e44bda337 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -337,12 +337,12 @@ class QEMUMachine:
> >   self._qmp.close()
> >   self._qmp_connection = None
> > -self._load_io_log()
> > -
> >   if self._qemu_log_file is not None:
> >   self._qemu_log_file.close()
> >   self._qemu_log_file = None
> > +self._load_io_log()
> > +
> 
> 
> IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being
> called before `self._load_io_log()` but a future change can make this
> condition unmet again. Maybe you could document that in the code. Or change
> the `_load_io_log()` to close the log file if it is open (also document it
> in code).
> 
> - Wainer
>

I'm glad you see this is needed... and then something else.  I'll investigate
making this more robust as time allows it.

Question is: do you ack/nack this change?

Cheers,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v4 03/24] python: create qemu packages

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:35PM -0500, John Snow wrote:
> move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
> directives across the tree.
> 
> This is done to create a PEP420 namespace package, in which we may
> create subpackages. To do this, the namespace directory ("qemu") should
> not have any modules in it. Those files will go into new 'machine' and 'qmp'
> subpackages instead.
> 
> Implement machine/__init__.py making the top-level classes and functions
> from its various modules available directly inside the package. Change
> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> library classes are available directly from "qemu.qmp" instead of
> "qemu.qmp.qmp".
> 
> Signed-off-by: John Snow 
> 
> 
> ---
> 
> Note for reviewers: in the next patch, I add a utils sub-package and
> move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
> way, we can squash it in here if we want, or just leave it as its own
> follow-up patch, I am just leaving it as something that will be easy to
> un-do or change for now.

IMO this is fine as it is.

> 
> Signed-off-by: John Snow 
> ---
>  python/{qemu => }/.isort.cfg|  0
>  python/qemu/__init__.py | 11 --
>  python/qemu/{ => machine}/.flake8   |  0
>  python/qemu/machine/__init__.py | 41 +
>  python/qemu/{ => machine}/accel.py  |  0
>  python/qemu/{ => machine}/console_socket.py |  0
>  python/qemu/{ => machine}/machine.py| 16 +---
>  python/qemu/{ => machine}/pylintrc  |  0
>  python/qemu/{ => machine}/qtest.py  |  3 +-
>  python/qemu/{qmp.py => qmp/__init__.py} | 12 +-
>  tests/acceptance/boot_linux.py  |  3 +-
>  tests/qemu-iotests/300  |  4 +-
>  tests/qemu-iotests/iotests.py   |  2 +-
>  tests/vm/basevm.py  |  3 +-
>  14 files changed, 70 insertions(+), 25 deletions(-)
>  rename python/{qemu => }/.isort.cfg (100%)
>  delete mode 100644 python/qemu/__init__.py
>  rename python/qemu/{ => machine}/.flake8 (100%)
>  create mode 100644 python/qemu/machine/__init__.py
>  rename python/qemu/{ => machine}/accel.py (100%)
>  rename python/qemu/{ => machine}/console_socket.py (100%)
>  rename python/qemu/{ => machine}/machine.py (98%)
>  rename python/qemu/{ => machine}/pylintrc (100%)
>  rename python/qemu/{ => machine}/qtest.py (99%)
>  rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
> 
> diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
> similarity index 100%
> rename from python/qemu/.isort.cfg
> rename to python/.isort.cfg
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> deleted file mode 100644
> index 4ca06c34a41..000
> --- a/python/qemu/__init__.py
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -# QEMU library
> -#
> -# Copyright (C) 2015-2016 Red Hat Inc.
> -# Copyright (C) 2012 IBM Corp.
> -#
> -# Authors:
> -#  Fam Zheng 
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.  See
> -# the COPYING file in the top-level directory.
> -#
> diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
> similarity index 100%
> rename from python/qemu/.flake8
> rename to python/qemu/machine/.flake8
> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
> new file mode 100644
> index 000..27b0b19abd3
> --- /dev/null
> +++ b/python/qemu/machine/__init__.py
> @@ -0,0 +1,41 @@
> +"""
> +QEMU development and testing library.
> +
> +This library provides a few high-level classes for driving QEMU from a
> +test suite, not intended for production use.
> +
> +- QEMUMachine: Configure and Boot a QEMU VM
> + - QEMUQtestMachine: VM class, with a qtest socket.
> +
> +- QEMUQtestProtocol: Connect to, send/receive qtest messages.
> +
> +- list_accel: List available accelerators
> +- kvm_available: Probe for KVM support
> +- tcg_available: Probe for TCG support
> +"""
> +
> +# Copyright (C) 2020 John Snow for Red Hat Inc.
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  John Snow 
> +#  Fam Zheng 
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +from .accel import kvm_available, list_accel, tcg_available
> +from .machine import QEMUMachine
> +from .qtest import QEMUQtestMachine, QEMUQtestProtocol
> +
> +
> +__all__ = (
> +'list_accel',
> +'kvm_available',
> +'tcg_available',
> +'QEMUMachine',
> +'QEMUQtestProtocol',
> +'QEMUQtestMachine',
> +)
> diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
> similarity index 100%
> rename from python/qemu/accel.py
> rename to python/qemu/machine/accel.py
> diff --git a/python/qemu/console_socket.py 
> b/python/qemu/machine/console_socket.py
> similarity index 100%
> rename from python/qemu/console_socket.py
> rename to python/qemu/machi

Re: [PATCH v4 02/24] iotests/297: add --namespace-packages to mypy arguments

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:34PM -0500, John Snow wrote:
> mypy is kind of weird about how it handles imports. For legacy reasons,
> it won't load PEP 420 namespaces, because of logic implemented prior to
> that becoming a standard.
> 
> So, if you plan on using any, you have to pass
> --namespace-packages. Alright, fine.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/297 | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 01/24] python/console_socket: avoid one-letter variable

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:33PM -0500, John Snow wrote:
> Fixes pylint warnings.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/console_socket.py | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 00/24] python: create installable package

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
> This series factors the python/qemu directory as an installable
> package. It does not yet actually change the mechanics of how any other
> python source in the tree actually consumes it (yet), beyond the import
> path. (some import statements change in a few places.)
> 
> git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
> (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
> 
> The primary motivation of this series is primarily to formalize our
> dependencies on mypy, flake8, isort, and pylint alongside versions that
> are known to work. It does this using the setup.cfg and setup.py
> files. It also adds explicitly pinned versions (using Pipfile.lock) of
> these dependencies that should behave in a repeatable and known way for
> developers and CI environments both. Lastly, it enables those CI checks
> such that we can enforce Python coding quality checks via the CI tests.
> 
> An auxiliary motivation is that this package is formatted in such a way
> that it COULD be uploaded to https://pypi.org/project/qemu and installed
> independently of qemu.git with `pip install qemu`, but that button
> remains *unpushed* and this series *will not* cause any such
> releases. We have time to debate finer points like API guarantees and
> versioning even after this series is merged.
> 
> Some other things this enables that might be of interest:
> 
> With the python tooling as a proper package, you can install this
> package in editable or production mode to a virtual environment, your
> local user environment, or your system packages. The primary benefit of
> this is to gain access to QMP tooling regardless of CWD, without needing
> to battle sys.path (and confounding other python analysis tools).
> 
> For example: when developing, you may go to qemu/python/ and run `make
> venv` followed by `pipenv shell` to activate a virtual environment that
> contains the qemu python packages. These packages will always reflect
> the current version of the source files in the tree. When you are
> finished, you can simply exit the shell (^d) to remove these packages
> from your python environment.
> 
> When not developing, you could install a version of this package to your
> environment outright to gain access to the QMP and QEMUMachine classes
> for lightweight scripting and testing by using pip: "pip install [--user] ."
> 
> TESTING THIS SERIES:
> 
> First of all, nothing should change. Without any intervention,
> everything should behave exactly as it was before. The only new
> information here comes from how to interact with and run the linters
> that will be enforcing code quality standards in this subdirectory.
> 
> To test those, CD to qemu/python first, and then:
> 
> 1. Try "make venv && pipenv shell" to get a venv with the package
> installed to it in editable mode. Ctrl+d exits this venv shell. While in
> this shell, any python script that uses "from qemu.[qmp|machine] import
> ..." should work correctly regardless of where the script is, or what
> your CWD is.
>

Ack here, works as expected.

> You will need Python 3.6 installed on your system to do this step. For
> Fedora: "dnf install python3.6" will do the trick.
>

You may have explained this before, so forgive me asking again.  Why
is this dependent on a given Python version, and not a *minimum*
Python version? Are the checkers themselves susceptible to different
behavior depending on the Python version used?  If so, any hint on the
strategy for developing with say, Python 3.8, and then having issues
caught only on Python 3.6?

> 2. Try "make check" while still in the shell to run the Python linters
> using the venv built in the previous step. This will pull some packages
> from PyPI and run pytest, which will in turn execute mypy, flake8, isort
> and pylint with the correct arguments.
>

Works as expected.  I'll provide more feedback at the specific patches.

> 3. Having exited the shell from above, try "make venv-check". This will
> create and update the venv if needed, then run 'make check' within the
> context of that shell. It should pass as long as the above did.
>

If this makes into a documentation (or on a v5), you may just want to
tell users to run "deactivate" instead of exiting the shell completely.

> 4. Still outside of the venv, you may try running "make check". This
> will not install anything, but unless you have the right Python
> dependencies installed, these tests may fail for you. You might try
> using "pip install --user .[devel]" to install the development packages
> needed to run the tests successfully to your local user's python
> environment. Once done, you will probably want to "pip uninstall qemu"
> to remove that package to avoid it interfering with other things.
>

This is good info for completeness, but I wonder if "make check"
should exist at all.  If it's a requirement for "make check-venv", t

Re: [PATCH 2/6] Python: expose QEMUMachine's temporary directory

[Cleber Rosa]

On Fri, Feb 12, 2021 at 12:35:26AM +0100, Philippe Mathieu-Daudé wrote:
> On 2/11/21 11:01 PM, Cleber Rosa wrote:
> > Each instance of qemu.machine.QEMUMachine currently has a "test
> > directory", which may not have any relation to a "test", and it's
> > really a temporary directory.
> > 
> > Users instantiating the QEMUMachine class will be able to set the
> > location of the directory that will *contain* the QEMUMachine unique
> > temporary directory, so that parameter name has been changed from
> > test_dir to base_temp_dir.
> > 
> > A property has been added to allow users to access it without using
> > private attributes, and with that, the directory is created on first
> > use of the property.
> > 
> > Signed-off-by: Cleber Rosa 
> > ---
> >  python/qemu/machine.py | 24 
> >  python/qemu/qtest.py   |  6 +++---
> >  tests/acceptance/virtio-gpu.py |  2 +-
> >  tests/qemu-iotests/iotests.py  |  2 +-
> >  4 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 6e44bda337..b379fcbe72 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -84,7 +84,7 @@ class QEMUMachine:
> >   args: Sequence[str] = (),
> >   wrapper: Sequence[str] = (),
> >   name: Optional[str] = None,
> > - test_dir: str = "/var/tmp",
> > + base_temp_dir: str = "/var/tmp",
> 
> Not this patch fault, but I see we use /var/tmp since commit
> 66613974468 ("scripts: refactor the VM class in iotests for reuse").
> Can we use an OS agnostic method to get temp storage directory instead?
> 

Sounds like a reasonable thing to do.  I do remember a few issues with
Python's tempfile.gettempdir() returning '/tmp' on most Linux/Unix,
dur to the fact that '/tmp' is a tmpfs filesystem on most modern Linux
systems.

Anyway, I agree that hardcoding '/var/tmp' needs to be reconsidered.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

[PATCH 5/6] Acceptance Tests: distinguish between temp and logs dir

[Cleber Rosa]

Logs can be very important to debug issues, and currently QEMUMachine
instances will remove logs that are created under the temporary
directories.

With this change, the stdout and stderr generated by the QEMU process
started by QEMUMachine will always be kept along the test results
directory.

Signed-off-by: Cleber Rosa 
---
 python/qemu/machine.py| 16 ++--
 tests/acceptance/avocado_qemu/__init__.py |  3 ++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index b379fcbe72..1f4119e2b4 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -89,7 +89,8 @@ class QEMUMachine:
  socket_scm_helper: Optional[str] = None,
  sock_dir: Optional[str] = None,
  drain_console: bool = False,
- console_log: Optional[str] = None):
+ console_log: Optional[str] = None,
+ log_dir: Optional[str] = None):
 '''
 Initialize a QEMUMachine
 
@@ -103,6 +104,7 @@ class QEMUMachine:
 @param sock_dir: where to create socket (defaults to base_temp_dir)
 @param drain_console: (optional) True to drain console socket to buffer
 @param console_log: (optional) path to console log file
+@param log_dir: where to create and keep log files
 @note: Qemu process is not started until launch() is used.
 '''
 # Direct user configuration
@@ -114,6 +116,7 @@ class QEMUMachine:
 self._name = name or "qemu-%d" % os.getpid()
 self._base_temp_dir = base_temp_dir
 self._sock_dir = sock_dir or self._base_temp_dir
+self._log_dir = log_dir
 self._socket_scm_helper = socket_scm_helper
 
 if monitor_address is not None:
@@ -303,7 +306,7 @@ class QEMUMachine:
 return args
 
 def _pre_launch(self) -> None:
-self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log")
+self._qemu_log_path = os.path.join(self.log_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
 if self._console_set:
@@ -752,3 +755,12 @@ class QEMUMachine:
 self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
   dir=self._base_temp_dir)
 return self._temp_dir
+
+@property
+def log_dir(self) -> str:
+"""
+Returns a directory to be used for writing logs
+"""
+if self._log_dir is None:
+return self.temp_dir
+return self._log_dir
diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 94b78fd7c8..ac9be1eb66 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -173,9 +173,10 @@ class Test(avocado.Test):
 def _new_vm(self, name, *args):
 self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
 vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
- sock_dir=self._sd.name)
+ sock_dir=self._sd.name, log_dir=self.logdir)
 self.log.debug('QEMUMachine "%s" created', name)
 self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir)
+self.log.debug('QEMUMachine "%s" log_dir: %s', name, vm.log_dir)
 if args:
 vm.add_args(*args)
 return vm
-- 
2.25.4

[PATCH 4/6] Acceptance Tests: log information when creating QEMUMachine

[Cleber Rosa]

Including its base temporary directory, given that information useful
for debugging can be put there.

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index b7ab836455..94b78fd7c8 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -170,10 +170,12 @@ class Test(avocado.Test):
 if self.qemu_bin is None:
 self.cancel("No QEMU binary defined or found in the build tree")
 
-def _new_vm(self, *args):
+def _new_vm(self, name, *args):
 self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
 vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
  sock_dir=self._sd.name)
+self.log.debug('QEMUMachine "%s" created', name)
+self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir)
 if args:
 vm.add_args(*args)
 return vm
@@ -186,7 +188,7 @@ class Test(avocado.Test):
 if not name:
 name = str(uuid.uuid4())
 if self._vms.get(name) is None:
-self._vms[name] = self._new_vm(*args)
+self._vms[name] = self._new_vm(name, *args)
 if self.machine is not None:
 self._vms[name].set_machine(self.machine)
 return self._vms[name]
-- 
2.25.4

[PATCH 2/6] Python: expose QEMUMachine's temporary directory

[Cleber Rosa]

Each instance of qemu.machine.QEMUMachine currently has a "test
directory", which may not have any relation to a "test", and it's
really a temporary directory.

Users instantiating the QEMUMachine class will be able to set the
location of the directory that will *contain* the QEMUMachine unique
temporary directory, so that parameter name has been changed from
test_dir to base_temp_dir.

A property has been added to allow users to access it without using
private attributes, and with that, the directory is created on first
use of the property.

Signed-off-by: Cleber Rosa 
---
 python/qemu/machine.py | 24 
 python/qemu/qtest.py   |  6 +++---
 tests/acceptance/virtio-gpu.py |  2 +-
 tests/qemu-iotests/iotests.py  |  2 +-
 4 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337..b379fcbe72 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -84,7 +84,7 @@ class QEMUMachine:
  args: Sequence[str] = (),
  wrapper: Sequence[str] = (),
  name: Optional[str] = None,
- test_dir: str = "/var/tmp",
+ base_temp_dir: str = "/var/tmp",
  monitor_address: Optional[SocketAddrT] = None,
  socket_scm_helper: Optional[str] = None,
  sock_dir: Optional[str] = None,
@@ -97,10 +97,10 @@ class QEMUMachine:
 @param args: list of extra arguments
 @param wrapper: list of arguments used as prefix to qemu binary
 @param name: prefix for socket and log file names (default: qemu-PID)
-@param test_dir: where to create socket and log file
+@param base_temp_dir: default location where temporary files are 
created
 @param monitor_address: address for QMP monitor
 @param socket_scm_helper: helper program, required for send_fd_scm()
-@param sock_dir: where to create socket (overrides test_dir for sock)
+@param sock_dir: where to create socket (defaults to base_temp_dir)
 @param drain_console: (optional) True to drain console socket to buffer
 @param console_log: (optional) path to console log file
 @note: Qemu process is not started until launch() is used.
@@ -112,8 +112,8 @@ class QEMUMachine:
 self._wrapper = wrapper
 
 self._name = name or "qemu-%d" % os.getpid()
-self._test_dir = test_dir
-self._sock_dir = sock_dir or self._test_dir
+self._base_temp_dir = base_temp_dir
+self._sock_dir = sock_dir or self._base_temp_dir
 self._socket_scm_helper = socket_scm_helper
 
 if monitor_address is not None:
@@ -303,9 +303,7 @@ class QEMUMachine:
 return args
 
 def _pre_launch(self) -> None:
-self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
-  dir=self._test_dir)
-self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
+self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
 if self._console_set:
@@ -744,3 +742,13 @@ class QEMUMachine:
 file=self._console_log_path,
 drain=self._drain_console)
 return self._console_socket
+
+@property
+def temp_dir(self) -> str:
+"""
+Returns a temporary directory to be used for this machine
+"""
+if self._temp_dir is None:
+self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
+  dir=self._base_temp_dir)
+return self._temp_dir
diff --git a/python/qemu/qtest.py b/python/qemu/qtest.py
index 39a0cf62fe..78b97d13cf 100644
--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -112,14 +112,14 @@ class QEMUQtestMachine(QEMUMachine):
  binary: str,
  args: Sequence[str] = (),
  name: Optional[str] = None,
- test_dir: str = "/var/tmp",
+ base_temp_dir: str = "/var/tmp",
  socket_scm_helper: Optional[str] = None,
  sock_dir: Optional[str] = None):
 if name is None:
 name = "qemu-%d" % os.getpid()
 if sock_dir is None:
-sock_dir = test_dir
-super().__init__(binary, args, name=name, test_dir=test_dir,
+sock_dir = base_temp_dir
+super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir)
 self._qtest: Optional[QEMUQtestProtocol] = None
diff --git a/tests/acceptance/virtio-gpu.py b/tests/accepta

[PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

Closing a file that is open for writing, and then reading from it
sounds like a better idea than the opposite, given that the content
will be flushed.

Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
Signed-off-by: Cleber Rosa 
---
 python/qemu/machine.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 7a40f4604b..6e44bda337 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -337,12 +337,12 @@ class QEMUMachine:
 self._qmp.close()
 self._qmp_connection = None
 
-self._load_io_log()
-
 if self._qemu_log_file is not None:
 self._qemu_log_file.close()
 self._qemu_log_file = None
 
+self._load_io_log()
+
 self._qemu_log_path = None
 
 if self._temp_dir is not None:
-- 
2.25.4

[PATCH 0/6] Python / Acceptance Tests: improve logging

[Cleber Rosa]

The location and amount of information kept while using QEMUMachine in
Acceptance Tests is currently not optimal.

This improves the situation by using the Test's log directory (an
Avocado standard feature) as the default location to keep logs,
instead of the temporary directory currently used.

Users will be able to find "qemu-$PID.log" files under the test log
directories, containing all the stdout/stderr generated by the QEMU
binary.

Cleber Rosa (6):
  Python: close the log file kept by QEMUMachine before reading it
  Python: expose QEMUMachine's temporary directory
  Acceptance Tests: use the job work directory for created VMs
  Acceptance Tests: log information when creating QEMUMachine
  Acceptance Tests: distinguish between temp and logs dir
  tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

 python/qemu/machine.py| 42 +--
 python/qemu/qtest.py  |  6 ++--
 tests/acceptance/avocado_qemu/__init__.py | 10 --
 tests/acceptance/virtio-gpu.py|  5 +--
 tests/qemu-iotests/iotests.py |  2 +-
 5 files changed, 45 insertions(+), 20 deletions(-)

-- 
2.25.4

[PATCH 6/6] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Cleber Rosa]

At location already prepared for keeping the test's log files.

While at it, log info about its location (in the main test log
file), instead of printing it out.

Reference: 
https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/virtio-gpu.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 8d689eb820..ab1a4c1a71 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -119,10 +119,11 @@ class VirtioGPUx86(Test):
 os.set_inheritable(vug_sock.fileno(), True)
 
 self._vug_log_path = os.path.join(
-self.vm.temp_dir, "vhost-user-gpu.log"
+self.logdir, "vhost-user-gpu.log"
 )
 self._vug_log_file = open(self._vug_log_path, "wb")
-print(self._vug_log_path)
+self.log.info('Complete vhost-user-gpu.log file can be '
+  'found at %s', self._vug_log_path)
 
 vugp = subprocess.Popen(
 [vug, "--virgl", "--fd=%d" % vug_sock.fileno()],
-- 
2.25.4

[PATCH 3/6] Acceptance Tests: use the job work directory for created VMs

[Cleber Rosa]

The QEMUMachine uses a base temporary directory for all temporary
needs.  By setting it to the Avocado's workdir, it's possible to
keep the temporary files during debugging sessions much more
easily by setting the "--keep-tmp" command line option.

Reference: 
https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.workdir
Reference: 
https://avocado-framework.readthedocs.io/en/85.0/config/index.html#run-keep-tmp
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index bf54e419da..b7ab836455 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -172,7 +172,8 @@ class Test(avocado.Test):
 
 def _new_vm(self, *args):
 self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
-vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
+vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
+ sock_dir=self._sd.name)
 if args:
 vm.add_args(*args)
 return vm
-- 
2.25.4

Re: [PULL 5/7] tests.acceptance: adds simple migration test

[Cleber Rosa]

On Tue, Nov 03, 2020 at 11:40:30AM +0100, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On Fri, Feb 22, 2019 at 8:42 PM Cleber Rosa  wrote:
> >
> > From: Caio Carrara 
> >
> > This change adds the simplest possible migration test. Beyond the test
> > purpose itself it's also useful to exercise the multi virtual machines
> > capabilities from base avocado qemu test class.
> >
> > Signed-off-by: Cleber Rosa 
> > Signed-off-by: Caio Carrara 
> > Reviewed-by: Cleber Rosa 
> > Reviewed-by: Wainer dos Santos Moschetta 
> > Message-Id: <20190212193855.13223-3-ccarr...@redhat.com>
> > Signed-off-by: Cleber Rosa 
> > ---
> >  tests/acceptance/migration.py | 53 +++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 tests/acceptance/migration.py
> >
> > diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
> > new file mode 100644
> > index 00..6115cf6c24
> > --- /dev/null
> > +++ b/tests/acceptance/migration.py
> > @@ -0,0 +1,53 @@
> > +# Migration test
> > +#
> > +# Copyright (c) 2019 Red Hat, Inc.
> > +#
> > +# Authors:
> > +#  Cleber Rosa 
> > +#  Caio Carrara 
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > +# later.  See the COPYING file in the top-level directory.
> > +
> > +
> > +from avocado_qemu import Test
> > +
> > +from avocado.utils import network
> > +from avocado.utils import wait
> > +
> > +
> > +class Migration(Test):
> > +"""
> > +:avocado: enable
> > +"""
> > +
> > +timeout = 10
> > +
> > +@staticmethod
> > +def migration_finished(vm):
> > +return vm.command('query-migrate')['status'] in ('completed', 
> > 'failed')
> > +
> > +def _get_free_port(self):
> > +port = network.find_free_port()
> > +if port is None:
> > +self.cancel('Failed to find a free port')
> > +return port
> 
> This method doesn't seem to work when running with -j2: 2 tests started
> with different arch configurations get the same port... Is this a known issue?
>

It's not bullet proof, but it seems to be quite safe... This is what I've tried:

 $ ./tests/venv/bin/avocado run --test-runner=nrunner 
--nrunner-max-parallel-tasks=10 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost{,,,}
 
JOB ID : 377957f4a16fbc2c6a6f6d9ae225c61af86bd570
JOB LOG: 
/home/cleber/avocado/job-results/job-2020-11-03T10.24-377957f/job.log
 (02/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (06/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (01/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (05/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (03/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (10/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (07/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (09/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (04/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (08/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: 
STARTED
 (02/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.46 s)
 (06/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.60 s)
 (01/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.51 s)
 (03/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.49 s)
 (05/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.52 s)
 (10/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.51 s)
 (07/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.52 s)
 (09/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.53 s)
 (04/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.49 s)
 (08/20) 
tests/acceptance/migration.py:Migration.test_migration_with_tcp_localhost: PASS 
(0.56 s)
...
RESULTS: PASS 20 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
CANCEL 0
JOB HTML   : 
/home/clebe

Re: [PATCH v3 15/15] python/qemu: add qemu package itself to pipenv

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:55PM -0400, John Snow wrote:
> This adds the python qemu packages themselves to the pipenv manifest.
> 'pipenv sync' will create a virtual environment sufficient to use the SDK.
> 'pipenv sync --dev' will create a virtual environment sufficient to use
> and test the SDK (with pylint, mypy, isort, flake8, etc.)
> 
> The qemu packages are installed in 'editable' mode; all changes made to
> the python package inside the git tree will be reflected in the
> installed package without reinstallation. This includes changes made
> via git pull and so on.
> 
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 14/15] python/qemu: add isort to pipenv

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:54PM -0400, John Snow wrote:
> isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
> certain "from ..." clauses that are not related to imports.
> 
> Require 5.0.5 or greater.
> 
> isort can be run with 'isort -c qemu' from the python root.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 13/15] python: move .isort.cfg into setup.cfg

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:53PM -0400, John Snow wrote:
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 10/15] python: Add flake8 to pipenv

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:50PM -0400, John Snow wrote:
> flake8 3.5.x does not support the --extend-ignore syntax used in the
> .flake8 file to gracefully extend default ignores, so 3.6.x is our
> minimum requirement. There is no known upper bound.
> 
> flake8 can be run from the python/ directory with no arguments.
> 
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 12/15] python: add mypy to pipenv

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:52PM -0400, John Snow wrote:
> 0.730 appears to be about the oldest version that works with the
> features we want, including nice human readable output (to make sure
> iotest 297 passes), and type-parameterized Popen generics.
> 
> 0.770, however, supports adding 'strict' to the config file, so require
> at least 0.770.
> 
> Now that we are checking a namespace package, we need to tell mypy to
> allow PEP420 namespaces, so modify the mypy config as part of the move.
> 
> mypy can now be run from the python root by typing 'mypy qemu'.
> 
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 11/15] python: move mypy.ini into setup.cfg

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:51PM -0400, John Snow wrote:
> mypy supports reading its configuration values from a central project
> configuration file; do so.
> 
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 09/15] python: move flake8 config to setup.cfg

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:49PM -0400, John Snow wrote:
> Update the comment concerning the flake8 exception to match commit
> 42c0dd12, whose commit message stated:
> 
> A note on the flake8 exception: flake8 will warn on *any* bare except,
> but pylint's is context-aware and will suppress the warning if you
> re-raise the exception.
> 
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 08/15] python: add pylint to pipenv

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:48PM -0400, John Snow wrote:
> We are specifying >= pylint 2.6.x for two reasons:
> 
> 1. For setup.cfg support, added in pylint 2.5.x
> 2. To clarify that we are using a version that has incompatibly dropped
> bad-whitespace checks.
> 
> Signed-off-by: John Snow 
> ---

I'm not a huge fan of this level of verbosity that pipenv generates,
but at the same time, I've been bitten too many times by not providing
the entire dep tree in a "requirements.txt"-like style.  And it is
what pipenv uses, so there's no way around that.

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 07/15] python: move pylintrc into setup.cfg

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:47PM -0400, John Snow wrote:
> Delete the empty settings now that it's sharing a home with settings for
> other tools.
> 
> pylint can now be run from this folder as "pylint qemu".
> Signed-off-by: John Snow 
> ---

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 06/15] python: add pylint import exceptions

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:46PM -0400, John Snow wrote:
> Pylint 2.5.x and 2.6.x have regressions that make import checking
> inconsistent, see:
> 
> https: //github.com/PyCQA/pylint/issues/3609
> https: //github.com/PyCQA/pylint/issues/3624
> https: //github.com/PyCQA/pylint/issues/3651
>

Are these whitespaces on purpose?

> Pinning to 2.4.4 is worse, because it mandates versions of shared
> dependencies that are too old for features we want in isort and mypy.
> Oh well.
> 
> Signed-off-by: John Snow 

Other than that,

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 05/15] python: Add pipenv support

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:45PM -0400, John Snow wrote:
> pipenv is a tool used for managing virtual environments with pinned,
> explicit dependencies. It is used for precisely recreating python
> virtual environments.
> 
> pipenv uses two files to do this:
> 
> (1) Pipfile, which is similar in purpose and scope to what setup.py
> lists. It specifies the requisite minimum to get a functional
> environment for using this package.
> 
> (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> requirements.txt`. It specifies a canonical virtual environment used for
> deployment or testing. This ensures that all users have repeatable
> results.
> 
> The primary benefit of using this tool is to ensure repeatable CI
> results with a known set of packages. Although I endeavor to support as
> many versions as I can, the fluid nature of the Python toolchain often
> means tailoring code for fairly specific versions.
> 
> Note that pipenv is *not* required to install or use this module; this is
> purely for the sake of repeatable testing by CI or developers.
> 
> Here, a "blank" pipfile is added with no dependencies, but specifies
> Python 3.6 for the virtual environment.
> 
> Pipfile will specify our version minimums, while Pipfile.lock specifies
> an exact loudout of packages that were known to operate correctly. This
> latter file provides the real value for easy setup of container images
> and CI environments.
> 
> Signed-off-by: John Snow 

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 04/15] python: add directory structure README.rst files

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:44PM -0400, John Snow wrote:
> Add short readmes to python/, python/qemu/, python/qemu/machine, and
> python/qemu/machine that explain the directory hierarchy. These readmes
> are visible when browsing the source on e.g. gitlab/github and are
> designed to help new developers/users quickly make sense of the source
> tree.
> 
> They are not designed for inclusion in a published manual.
> 
> Signed-off-by: John Snow 
> ---
>  python/README.rst  | 27 +++
>  python/qemu/README.rst |  8 
>  python/qemu/machine/README.rst |  9 +
>  python/qemu/qmp/README.rst |  9 +
>  4 files changed, 53 insertions(+)
>  create mode 100644 python/README.rst
>  create mode 100644 python/qemu/README.rst
>  create mode 100644 python/qemu/machine/README.rst
>  create mode 100644 python/qemu/qmp/README.rst
> 
> diff --git a/python/README.rst b/python/README.rst
> new file mode 100644
> index 00..ff40e4c931
> --- /dev/null
> +++ b/python/README.rst
> @@ -0,0 +1,27 @@
> +QEMU Python Tooling
> +===
> +
> +This directory houses Python tooling used by the QEMU project to build,
> +configure, and test QEMU. It is organized by namespace (``qemu``), and
> +then by package (``qemu/machine``, ``qemu/qmp``).
> +
> +``setup.py`` is used by ``pip`` to install this tooling to the current
> +environment. You will generally invoke it by doing one of the following:
> +
> +1. ``pip3 install .`` will install these packages to your current
> +   environment. If you are inside a virtual environment, they will
> +   install there. If you are not, it will attempt to install to the
> +   global environment, which is not recommended.
> +
> +2. ``pip3 install --user .`` will install these packages to your user's
> +   local python packages. If you are inside of a virtual environment,
> +   this will fail.
> +
> +If you amend the ``-e`` argument, pip will install in "editable" mode;
> +which installs a version of the package that uses symlinks to these
> +files, such that the package always reflects the latest version in your
> +git tree.
> +

It actually uses *egg-link* files, which are not what people will
understand by "symlinks".

> +See `Installing packages using pip and virtual environments
> +`_
> +for more information.
> diff --git a/python/qemu/README.rst b/python/qemu/README.rst
> new file mode 100644
> index 00..31209c80a5
> --- /dev/null
> +++ b/python/qemu/README.rst
> @@ -0,0 +1,8 @@
> +QEMU Python Namespace
> +=
> +
> +This directory serves as the root of a `Python PEP 420 implicit
> +namespace package <`_.
> +
> +Each directory below is assumed to be an installable Python package that
> +is available under the ``qemu.`` namespace.
> diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
> new file mode 100644
> index 00..73ad23c501
> --- /dev/null
> +++ b/python/qemu/machine/README.rst
> @@ -0,0 +1,9 @@
> +qemu.machine Package
> +
> +
> +This package provides core utilities used for testing and debugging
> +QEMU. It is used by the iotests, vm tests, and several other utilities
> +in the ./scripts directory. It is not a fully-fledged SDK and it is
> +subject to change at any time.
> +

I'm not sure if you intend to list all test types that use this, but
the acceptance tests also do use them.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v3 03/15] python: add VERSION file

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:43PM -0400, John Snow wrote:
> Python infrastructure as it exists today is not capable reliably of
> single-sourcing a package version from a parent directory. The authors
> of pip are working to correct this, but as of today this is not possible
> to my knowledge.
> 
> The problem is that when using pip to build and install a python
> package, it copies files over to a temporary directory and performs its
> build there. This loses access to any information in the parent
> directory, including git itself.
> 
> Further, Python versions have a standard (PEP 440) that may or may not
> follow QEMU's versioning. In general, it does; but naturally QEMU does
> not follow PEP 440. To avoid any automatically-generated conflict, a
> manual version file is preferred.
> 
> 
> I am proposing:
> 
> - Python tooling follows the QEMU version, indirectly, but with a major
>   version of 0 to indicate that the API is not expected to be
>   stable. This would mean version 0.5.2.0, 0.5.1.1, 0.5.3.0, etc.
> 
> - In the event that a Python package needs to be updated independently
>   of the QEMU version, a pre-release alpha version should be preferred,
>   but *only* after inclusion to the qemu development or stable branches.
> 
>   e.g. 0.5.2.0a1, 0.5.2.0a2, and so on should be preferred prior to
>   5.2.0's release.
> 
> - The Python core tooling makes absolutely no version compatibility
>   checks or constraints. It *may* work with releases of QEMU from the
>   past or future, but it is not required to.
> 
>   i.e., "qemu.machine" will always remain in lock-step with QEMU.
> 
> - We reserve the right to split the qemu package into independently
>   versioned subpackages at a later date. This might allow for us to
>   begin versioning QMP independently from QEMU at a later date, if
>   we so choose.
> 
> 
> Implement this versioning scheme by adding a VERSION file and setting it
> to 0.5.2.0a1.
> 
> Signed-off-by: John Snow 

I'd rather have the version to be sync'd with QEMU, but, I understand
this is a more conservative approach that can maybe evolve into that.

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v3 02/15] python: add qemu package installer

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:42PM -0400, John Snow wrote:
> Add setup.cfg and setup.py, necessary for installing a package via
> pip. Add a rst document explaining the basics of what this package is
> for and who to contact for more information. This document will be used
> as the landing page for the package on PyPI.
> 
> I am not yet using a pyproject.toml style package manifest, because
> using pyproject.toml (and PEP-517) style packages means that pip is not
> able to install in "editable" or "develop" mode, which I consider
> necessary for the development of this package.
> 
> Use a light-weight setup.py instead.
> 
> Signed-off-by: John Snow 
> ---
>  python/PACKAGE.rst | 26 ++
>  python/setup.cfg   | 18 ++
>  python/setup.py| 23 +++
>  3 files changed, 67 insertions(+)
>  create mode 100644 python/PACKAGE.rst
>  create mode 100755 python/setup.cfg

I missed this earlier: setup.cfg does not need to have the execute bit
on.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v3 02/15] python: add qemu package installer

[Cleber Rosa]

On Wed, Oct 28, 2020 at 01:02:52PM -0400, John Snow wrote:
> On 10/28/20 11:10 AM, Cleber Rosa wrote:
> > > +mirror <https://gitlab.com/jsnow/qemu/-/tree/python>`_, but
> > > +contributions must be sent to the list for inclusion.
> > 
> > IMO it's not clear if this branch/mirror is your development work, a
> > staging area, etc.
> > 
> 
> Fair enough. jsnow/qemu/python is intended as a staging area for patches
> that have been vetted on-list.
> 
> jsnow/qemu/master is a lazily-updated mirror. (I tend to update it every day
> as part of my development process, but there are days I don't write code.)
> 
> jsnow/qemu/python-* is development work; review branches, etc.
> 
> I'll try to rephrase this a bit. What I want to communicate:
> 
> - This package exists as a subfolder of a larger project
> - I am responsible for maintaining this package, but not for the larger
> project
> - Please contact *me* for problems with this package
> - Contributions should go through qemu-devel (I will gently redirect
> contributors who may send pull requests to the qemu devel list.)
>

OK, sounds good.  I'll look at the exact rewording on v+1.

> > > diff --git a/python/setup.cfg b/python/setup.cfg
> > > new file mode 100755
> > > index 00..12b99a796e
> > > --- /dev/null
> > > +++ b/python/setup.cfg
> > > @@ -0,0 +1,18 @@
> > > +[metadata]
> > > +name = qemu
> > > +maintainer = QEMU Developer Team
> > > +maintainer_email = qemu-de...@nongnu.org
> > > +url = https://www.qemu.org/
> > > +download_url = https://www.qemu.org/download/
> > > +description = QEMU Python Build, Debug and SDK tooling.
> > > +long_description = file:PACKAGE.rst
> > > +long_description_content_type = text/x-rst
> > > +classifiers =
> > > +Development Status :: 3 - Alpha
> > > +License :: OSI Approved :: GNU General Public License v2 (GPLv2)
> > > +Natural Language :: English
> > > +Operating System :: OS Independent
> > > +
> > 
> 
> Also ... licensing question, do we need *L*GPLv2, or does Python not have a
> "linking exception" problem?
> 
> I guess we can't really re-license these files anyway, so nevermind.
> 
> (I immediately regret asking this.)
>

I'll just pretend you never did.

> > I know the sky is the limit, but I miss the Python version classifier,
> > at least:
> > 
> >Programming Language :: Python :: 3 :: Only
> > 
> 
> Sure.
> 
> Wait, why can you specify Python as a language? Is it possible to have
> non-Python packages on PyPI?
> 
> *CONCERNED*
>

I guess it has to do with packages that can interact or serve other
languages.  Or, that are (partially) written in another language?

> > And optionally those:
> > 
> >Programming Language :: Python :: 3.6
> >Programming Language :: Python :: 3.7
> >Programming Language :: Python :: 3.8
> >Programming Language :: Python :: 3.9
> > 
> > Although it may be a good idea to add them along test jobs on those
> > specific Python versions.
> > 
> 
> Are these worth adding? I've got python_requires >= 3.6 down below. From my
> test of a blank package upload to PyPI, it seems to display prominently:
> 
> https://pypi.org/project/qemu/
> 
> Is there a tangible benefit that you are aware of?
>

AFAICT, the classifiers are about letting people search for packages
that match a given criteria.  It's all metadata, so the benefits are
not super tangible.  I've used those to keep track / document the
Python versions that I know the project has been actively tested on,
and that's the reason of my comment about (CI) test jobs.

> > > +[options]
> > > +python_requires = >= 3.6
> > > +packages = find_namespace:
> > > diff --git a/python/setup.py b/python/setup.py
> > > new file mode 100755
> > > index 00..e93d0075d1
> > > --- /dev/null
> > > +++ b/python/setup.py
> > > @@ -0,0 +1,23 @@
> > > +#!/usr/bin/env python3
> > > +"""
> > > +QEMU tooling installer script
> > > +Copyright (c) 2020 John Snow for Red Hat, Inc.
> > > +"""
> > > +
> > > +import setuptools
> > > +import pkg_resources
> > > +
> > > +
> > > +def main():
> > > +"""
> > > +QEMU tooling installer
> > > +"""
> > > +
> > > +# 
> > > https://medium.com/@daveshawley/safely-using

Re: [PATCH v3 01/15] python: create qemu packages

[Cleber Rosa]

On Wed, Oct 28, 2020 at 11:21:19AM -0400, John Snow wrote:
> On 10/28/20 10:46 AM, Cleber Rosa wrote:
> > On Tue, Oct 20, 2020 at 03:35:41PM -0400, John Snow wrote:
> > > move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
> > > directives across the tree.
> > > 
> > > This is done to create a PEP420 namespace package, in which we may
> > > create subpackages. To do this, the namespace directory ("qemu") should
> > > not have any modules in it. Those files will go into new 'machine' and 
> > > 'qmp'
> > > subpackages instead.
> > > 
> > > Implement machine/__init__.py making the top-level classes and functions
> > > from its various modules available directly inside the package. Change
> > > qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> > > library classes are available directly from "qemu.qmp" instead of
> > > "qemu.qmp.qmp".
> > > 
> > > Signed-off-by: John Snow 
> > > ---
> > >   python/{qemu => }/.isort.cfg|  0
> > >   python/qemu/__init__.py | 11 --
> > >   python/qemu/{ => machine}/.flake8   |  0
> > >   python/qemu/machine/__init__.py | 41 +
> > >   python/qemu/{ => machine}/accel.py  |  0
> > >   python/qemu/{ => machine}/console_socket.py |  0
> > >   python/qemu/{ => machine}/machine.py| 16 +---
> > >   python/qemu/{ => machine}/pylintrc  |  0
> > >   python/qemu/{ => machine}/qtest.py  |  3 +-
> > >   python/qemu/{qmp.py => qmp/__init__.py} | 12 +-
> > >   tests/acceptance/boot_linux.py  |  3 +-
> > >   tests/qemu-iotests/297  |  2 +-
> > >   tests/qemu-iotests/300  |  4 +-
> > >   tests/qemu-iotests/iotests.py   |  2 +-
> > >   tests/vm/basevm.py  |  3 +-
> > >   15 files changed, 71 insertions(+), 26 deletions(-)
> > >   rename python/{qemu => }/.isort.cfg (100%)
> > >   delete mode 100644 python/qemu/__init__.py
> > >   rename python/qemu/{ => machine}/.flake8 (100%)
> > >   create mode 100644 python/qemu/machine/__init__.py
> > >   rename python/qemu/{ => machine}/accel.py (100%)
> > >   rename python/qemu/{ => machine}/console_socket.py (100%)
> > >   rename python/qemu/{ => machine}/machine.py (98%)
> > >   rename python/qemu/{ => machine}/pylintrc (100%)
> > >   rename python/qemu/{ => machine}/qtest.py (99%)
> > >   rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
> > > 
> > > diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
> > > similarity index 100%
> > > rename from python/qemu/.isort.cfg
> > > rename to python/.isort.cfg
> > > diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> > > deleted file mode 100644
> > > index 4ca06c34a4..00
> > > --- a/python/qemu/__init__.py
> > > +++ /dev/null
> > > @@ -1,11 +0,0 @@
> > > -# QEMU library
> > > -#
> > > -# Copyright (C) 2015-2016 Red Hat Inc.
> > > -# Copyright (C) 2012 IBM Corp.
> > > -#
> > > -# Authors:
> > > -#  Fam Zheng 
> > > -#
> > > -# This work is licensed under the terms of the GNU GPL, version 2.  See
> > > -# the COPYING file in the top-level directory.
> > > -#
> > > diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
> > > similarity index 100%
> > > rename from python/qemu/.flake8
> > > rename to python/qemu/machine/.flake8
> > > diff --git a/python/qemu/machine/__init__.py 
> > > b/python/qemu/machine/__init__.py
> > > new file mode 100644
> > > index 00..27b0b19abd
> > > --- /dev/null
> > > +++ b/python/qemu/machine/__init__.py
> > > @@ -0,0 +1,41 @@
> > > +"""
> > > +QEMU development and testing library.
> > > +
> > > +This library provides a few high-level classes for driving QEMU from a
> > > +test suite, not intended for production use.
> > > +
> > > +- QEMUMachine: Configure and Boot a QEMU VM
> > > + - QEMUQtestMachine: VM class, with a qtest socket.
> > > +
> > > +- QEMUQtestProtocol: Connect to, send/receive qtest messages.
> > > +
> > > +- list_accel: List available accelerators
> > > +- kvm_available:

Re: [PATCH v3 02/15] python: add qemu package installer

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:42PM -0400, John Snow wrote:
> Add setup.cfg and setup.py, necessary for installing a package via
> pip. Add a rst document explaining the basics of what this package is
> for and who to contact for more information. This document will be used
> as the landing page for the package on PyPI.
> 
> I am not yet using a pyproject.toml style package manifest, because
> using pyproject.toml (and PEP-517) style packages means that pip is not
> able to install in "editable" or "develop" mode, which I consider
> necessary for the development of this package.
> 
> Use a light-weight setup.py instead.
> 
> Signed-off-by: John Snow 
> ---
>  python/PACKAGE.rst | 26 ++
>  python/setup.cfg   | 18 ++
>  python/setup.py| 23 +++
>  3 files changed, 67 insertions(+)
>  create mode 100644 python/PACKAGE.rst
>  create mode 100755 python/setup.cfg
>  create mode 100755 python/setup.py
> 
> diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
> new file mode 100644
> index 00..b82f32f489
> --- /dev/null
> +++ b/python/PACKAGE.rst
> @@ -0,0 +1,26 @@
> +QEMU Python Tooling
> +===
> +
> +This package provides QEMU tooling used by the QEMU project to build,
> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
> +to change at any time.
> +
> +Usage
> +-
> +
> +The ``qemu.qmp`` subpackage provides a library for communicating with
> +QMP servers. The ``qemu.machine`` subpackage offers rudimentary
> +facilities for launching and managing QEMU processes. Refer to each
> +pacakge's documentation

Typo here: s/pacakge/package/

> +(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
> +for more information.
> +
> +Contributing
> +
> +
> +This package is maintained by John Snow  as part of
> +the QEMU source tree. Contributions are welcome and follow the `QEMU
> +patch submission process
> +`_. There is a `Gitlab

Git*L*ab.  Given that we're also under a company named with two words,
better given them that :)

> +mirror `_, but
> +contributions must be sent to the list for inclusion.

IMO it's not clear if this branch/mirror is your development work, a
staging area, etc.

> diff --git a/python/setup.cfg b/python/setup.cfg
> new file mode 100755
> index 00..12b99a796e
> --- /dev/null
> +++ b/python/setup.cfg
> @@ -0,0 +1,18 @@
> +[metadata]
> +name = qemu
> +maintainer = QEMU Developer Team
> +maintainer_email = qemu-de...@nongnu.org
> +url = https://www.qemu.org/
> +download_url = https://www.qemu.org/download/
> +description = QEMU Python Build, Debug and SDK tooling.
> +long_description = file:PACKAGE.rst
> +long_description_content_type = text/x-rst
> +classifiers =
> +Development Status :: 3 - Alpha
> +License :: OSI Approved :: GNU General Public License v2 (GPLv2)
> +Natural Language :: English
> +Operating System :: OS Independent
> +

I know the sky is the limit, but I miss the Python version classifier,
at least:

  Programming Language :: Python :: 3 :: Only

And optionally those:

  Programming Language :: Python :: 3.6
  Programming Language :: Python :: 3.7
  Programming Language :: Python :: 3.8
  Programming Language :: Python :: 3.9

Although it may be a good idea to add them along test jobs on those
specific Python versions.

> +[options]
> +python_requires = >= 3.6
> +packages = find_namespace:
> diff --git a/python/setup.py b/python/setup.py
> new file mode 100755
> index 00..e93d0075d1
> --- /dev/null
> +++ b/python/setup.py
> @@ -0,0 +1,23 @@
> +#!/usr/bin/env python3
> +"""
> +QEMU tooling installer script
> +Copyright (c) 2020 John Snow for Red Hat, Inc.
> +"""
> +
> +import setuptools
> +import pkg_resources
> +
> +
> +def main():
> +"""
> +QEMU tooling installer
> +"""
> +
> +# 
> https://medium.com/@daveshawley/safely-using-setup-cfg-for-metadata-1babbe54c108
> +pkg_resources.require('setuptools>=39.2')

Getting back to the "test jobs on those specific Python versions" I
was really anxious that environments with Python 3.6 will fail to
have such a "recent" setuptools version.

CentOS 8 has that specific version, while Ubuntu 18.04 has version
39.0.  Ubuntu 20.04 has a recent enough version though.  Given that
all GitLab CI moved to 20.04, this should be safe.

- Cleber.

> +
> +setuptools.setup()
> +
> +
> +if __name__ == '__main__':
> +main()
> -- 
> 2.26.2
> 


signature.asc
Description: PGP signature

Re: [PATCH v3 01/15] python: create qemu packages

[Cleber Rosa]

On Tue, Oct 20, 2020 at 03:35:41PM -0400, John Snow wrote:
> move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
> directives across the tree.
> 
> This is done to create a PEP420 namespace package, in which we may
> create subpackages. To do this, the namespace directory ("qemu") should
> not have any modules in it. Those files will go into new 'machine' and 'qmp'
> subpackages instead.
> 
> Implement machine/__init__.py making the top-level classes and functions
> from its various modules available directly inside the package. Change
> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> library classes are available directly from "qemu.qmp" instead of
> "qemu.qmp.qmp".
> 
> Signed-off-by: John Snow 
> ---
>  python/{qemu => }/.isort.cfg|  0
>  python/qemu/__init__.py | 11 --
>  python/qemu/{ => machine}/.flake8   |  0
>  python/qemu/machine/__init__.py | 41 +
>  python/qemu/{ => machine}/accel.py  |  0
>  python/qemu/{ => machine}/console_socket.py |  0
>  python/qemu/{ => machine}/machine.py| 16 +---
>  python/qemu/{ => machine}/pylintrc  |  0
>  python/qemu/{ => machine}/qtest.py  |  3 +-
>  python/qemu/{qmp.py => qmp/__init__.py} | 12 +-
>  tests/acceptance/boot_linux.py  |  3 +-
>  tests/qemu-iotests/297  |  2 +-
>  tests/qemu-iotests/300  |  4 +-
>  tests/qemu-iotests/iotests.py   |  2 +-
>  tests/vm/basevm.py  |  3 +-
>  15 files changed, 71 insertions(+), 26 deletions(-)
>  rename python/{qemu => }/.isort.cfg (100%)
>  delete mode 100644 python/qemu/__init__.py
>  rename python/qemu/{ => machine}/.flake8 (100%)
>  create mode 100644 python/qemu/machine/__init__.py
>  rename python/qemu/{ => machine}/accel.py (100%)
>  rename python/qemu/{ => machine}/console_socket.py (100%)
>  rename python/qemu/{ => machine}/machine.py (98%)
>  rename python/qemu/{ => machine}/pylintrc (100%)
>  rename python/qemu/{ => machine}/qtest.py (99%)
>  rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
> 
> diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
> similarity index 100%
> rename from python/qemu/.isort.cfg
> rename to python/.isort.cfg
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> deleted file mode 100644
> index 4ca06c34a4..00
> --- a/python/qemu/__init__.py
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -# QEMU library
> -#
> -# Copyright (C) 2015-2016 Red Hat Inc.
> -# Copyright (C) 2012 IBM Corp.
> -#
> -# Authors:
> -#  Fam Zheng 
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.  See
> -# the COPYING file in the top-level directory.
> -#
> diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
> similarity index 100%
> rename from python/qemu/.flake8
> rename to python/qemu/machine/.flake8
> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
> new file mode 100644
> index 00..27b0b19abd
> --- /dev/null
> +++ b/python/qemu/machine/__init__.py
> @@ -0,0 +1,41 @@
> +"""
> +QEMU development and testing library.
> +
> +This library provides a few high-level classes for driving QEMU from a
> +test suite, not intended for production use.
> +
> +- QEMUMachine: Configure and Boot a QEMU VM
> + - QEMUQtestMachine: VM class, with a qtest socket.
> +
> +- QEMUQtestProtocol: Connect to, send/receive qtest messages.
> +
> +- list_accel: List available accelerators
> +- kvm_available: Probe for KVM support
> +- tcg_available: Probe for TCG support
> +"""
> +
> +# Copyright (C) 2020 John Snow for Red Hat Inc.
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  John Snow 
> +#  Fam Zheng 
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +from .accel import kvm_available, list_accel, tcg_available
> +from .machine import QEMUMachine
> +from .qtest import QEMUQtestMachine, QEMUQtestProtocol
> +
> +
> +__all__ = (
> +'list_accel',
> +'kvm_available',
> +'tcg_available',
> +'QEMUMachine',
> +'QEMUQtestProtocol',
> +'QEMUQtestMachine',
> +)

How far would this approach go?  I mean, should all future APIs be developed
inside module under "qemu/machine", say "qemu/machine/foo.py" and their main
functionality imported here?  Or do you see this as a temporary state?

IMO, this is hard to maintain, and is a very easy path to future
inconsistencies.

> diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
> similarity index 100%
> rename from python/qemu/accel.py
> rename to python/qemu/machine/accel.py
> diff --git a/python/qemu/console_socket.py 
> b/python/qemu/machine/console_socket.py
> similarity index 100%
> rename from python/qemu/console_socket.py
> rename to python/qemu/machine/console_socket.py
> diff --

Re: [PATCH v2 4/9] tests/acceptance/boot_linux: Expand SD card image to power of 2

[Cleber Rosa]

On Mon, Jul 13, 2020 at 08:32:04PM +0200, Philippe Mathieu-Daudé wrote:
> In few commits we won't allow SD card images with invalid size
> (not aligned to a power of 2). Prepare the tests: add the
> pow2ceil() and image_pow2ceil_expand() methods and resize the
> images (expanding) of the tests using SD cards.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Since v1: Addressed review comments
> - truncate -> expand reword (Alistair Francis)
> - expand after uncompress (Niek Linnenbank)
> ---
>  tests/acceptance/boot_linux_console.py | 27 +-
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py 
> b/tests/acceptance/boot_linux_console.py
> index b7e8858c2d..8f2a6aa8a4 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -28,6 +28,18 @@
>  except CmdNotFoundError:
>  P7ZIP_AVAILABLE = False
>  
> +# round up to next power of 2
> +def pow2ceil(x):
> +return 1 if x == 0 else 2**(x - 1).bit_length()
> +

Nitpick: turn the comment into a docstring.

Then, I was going to have a second nitpick about the method name, but
realized it was following qemu-common.h's implementation.

> +# expand file size to next power of 2
> +def image_pow2ceil_expand(path):
> +size = os.path.getsize(path)
> +size_aligned = pow2ceil(size)
> +if size != size_aligned:
> +with open(path, 'ab+') as fd:
> +        fd.truncate(size_aligned)
> +

Same nitpick comment about comment -> docstring here.

Either way,

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v2 3/9] tests/acceptance/boot_linux: Tag tests using a SD card with 'device:sd'

[Cleber Rosa]

On Mon, Jul 13, 2020 at 08:32:03PM +0200, Philippe Mathieu-Daudé wrote:
> Avocado tags are handy to automatically select tests matching
> the tags. Since these tests use a SD card, tag them.
> 
> We can run all the tests using a SD card at once with:
> 
>   $ avocado --show=app run -t u-boot tests/acceptance/
>   $ AVOCADO_ALLOW_LARGE_STORAGE=ok \
> avocado --show=app \
>   run -t device:sd tests/acceptance/
>   Fetching asset from 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
>   Fetching asset from 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic
>   Fetching asset from 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
>(1/3) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd: 
> PASS (19.56 s)
>(2/3) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic:
>  PASS (49.97 s)
>(3/3) 
> tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
>  PASS (20.06 s)
>   RESULTS: PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | 
> CANCEL 0
>   JOB TIME   : 90.02 s
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH 0/7] python: create installable package

[Cleber Rosa]

On Tue, Jun 02, 2020 at 08:15:16PM -0400, John Snow wrote:
> Based-on: 20200602214528.12107-1-js...@redhat.com
> 
> This series factors the python/qemu directory as an installable
> module. As a developer, you can install this to your virtual environment
> and then always have access to the classes contained within without
> needing to wrangle python import path problems.
>

First of all, major kudos for picking up this task.  It's so high in
importance to so many users (myself included) that I feel like I owe
you many truck loads of beers now. :)

> When developing, you could go to qemu/python/ and invoke `pipenv shell`
> to activate a virtual environment within which you could type `pip
> install -e .` to install a special development version of this package
> to your virtual environment. This package will always reflect the most
> recent version of the source files in the tree.
> 
> When not developing, you could install a version of this package to your
> environment outright to gain access to the QMP and QEMUMachine classes
> for lightweight scripting and testing.
> 
> This package is formatted in such a way that it COULD be uploaded to
> https://pypi.org/project/qemu and installed independently of qemu.git
> with `pip install qemu`, but of course that button remains unpushed.
> 
> There are a few major questions to answer first:
> 
> - What versioning scheme should we use? See patch 2.
> 
> - Should we use a namespace package or not?
>   - Namespaced: 'qemu.machine', 'qemu.monitor' etc may be separately
> versioned, packaged and distributed packages. Third party authors
> may register 'qemu.xxx' to create plugins within the namespace as
> they see fit.
> 
>   - Non-namespaced: 'qemu' is one giant glob package, packaged and
> versioned in unison. We control this package exclusively.
>

For simplicity sake, I'd suggest starting with non-namespaced
approach.  It should be easier to move to a namespaced package if the
need arises.  Also, there are many ways to extend Python code without
necessarily requiring third party authors to register their packages
according to a namespace.

In the Avocado project, we have been using setuptools entrypoints with
a reasonable level of success.  Anyone can have code under any
namespace whatsoever extending Avocado, as long as it register their
entrypoints.

> - How do we eliminate sys.path hacks from the rest of the QEMU tree?
>   (Background: sys.path hacks generally impede the function of static
>   code quality analysis tools like mypy and pylint.)
> 
>   - Simplest: parent scripts (or developer) needs to set PYTHONPATH.
> 
>   - Harder: Python scripts should all be written to assume package form,
> all tests and CI that use Python should execute within a VENV.
>

Having a venv is desirable, but it's not really necessary.  As long as
"python setup.py develop --user" is called, that user can access this
code without sys.path hacks.  And if the user chooses to use a venv,
it's just an extra step.

In the Avocado project, we have a `make develop` rule that does that
for the main setup.py file, and for all plugins we carry on the same
tree, which is similar in some regards to the "not at the project root
directory" situation here with "qemu/python/setup.py".

>   In either case, we lose the ability (for many scripts) to "just run" a
>   script out of the source tree if it depends on other QEMU Python
>   files. This is annoying, but as the complexity of the Python lib
>   grows, it is unavoidable.
>

Like I said before, we may introduce a "make develop"-like
requirement, but after that, I don't think we'll loose anything.
Also, I think this is just a sign of maturity.  We should be using
Python as it's inteded to be used, and sys.path hacks is not among
those.

>   In the VENV case, we at least establish a simple paradigm: the scripts
>   should work in their "installed" forms; and the rest of the build and
>   test infrastructure should use this VENV to automatically handle
>   dependencies and path requirements. This should allow us to move many
>   of our existing python scripts with "hidden" dependencies into a
>   proper python module hierarchy and test for regressions with mypy,
>   flake8, pylint, etc.
> 
>   (We could even establish e.g. Sphinx versions as a dependency for our
>   build kit here and make sure it's installed to the VENV.)
> 
>   Pros: Almost all scripts can be moved under python/qemu/* and checked
>   with CQA tools. imports are written the same no matter where you are
>   (Use the fully qualified names, e.g. qemu.core.qmp.QMPMessage).
>   Regressions in scripts are caught *much* faster.
> 
>   Downsides: Kind of annoying; most scripts now require you to install a
>   devkit forwarder (pip3 install --user .) or be inside of an activated
>   venv. Not too bad if you know python at all, but it's certainly less
>   plug-n-play.
> 
> - What's our backwards compatibility policy if we start shipping this?
> 
>   Proposed: Attempt t

Re: [PATCH for-5.0 v2 0/3] benchmark util

[Cleber Rosa]

On Tue, Nov 26, 2019 at 06:48:45PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is simple benchmarking utility, to generate performance
> comparison tables, like the following:
> 
> --  -  -  -
> backup-1   backup-2   mirror
> ssd -> ssd  0.43 +- 0.00   4.48 +- 0.06   4.38 +- 0.02
> ssd -> hdd  10.60 +- 0.08  10.69 +- 0.18  10.57 +- 0.05
> ssd -> nbd  33.81 +- 0.37  10.67 +- 0.17  10.07 +- 0.07
> --  -  -  -
> 
> This is a v2, as v1 was inside
>  "[RFC 00/24] backup performance: block_status + async"
> 
> I'll use this benchmark in other series, hope someone
> will like it.
> 
> Vladimir Sementsov-Ogievskiy (3):
>   python: add simplebench.py
>   python: add qemu/bench_block_job.py
>   python: add example usage of simplebench
> 
>  python/bench-example.py|  80 +
>  python/qemu/bench_block_job.py | 115 +
>  python/simplebench.py  | 128 +
>  3 files changed, 323 insertions(+)
>  create mode 100644 python/bench-example.py
>  create mode 100755 python/qemu/bench_block_job.py
>  create mode 100644 python/simplebench.py
> 
> -- 
> 2.18.0
> 

Hi Vladimir,

This looks interesting.

Do you think the execution of "test cases" in an "environment" are a
generic enough concept that could be reused (or reuse other system)?
My point is that it'd be nice to do the same thing say for the
acceptance tests, or any tests for that matter.  For instance, for
known parameters, we could record what's the time difference between
booting a guest with q35 or pc machine types and virtio-block or
virtio-scsi devices.

BTW, This reminded me of a IOzone[1] test runner / results analyzer:

  
https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/io/disk/iozone.py

I'm also cc'ing Lukáš Doktor, who has actively worked in something
similar.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v2 6/7] configure: allow disable of cross compilation containers

[Cleber Rosa]

On Wed, Dec 04, 2019 at 04:46:17PM +0100, Thomas Huth wrote:
> From: Alex Bennée 
> 
> Our docker infrastructure isn't quite as multiarch as we would wish so
> let's allow the user to disable it if they want. This will allow us to
> use still run check-tcg on non-x86 CI setups.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Stefan Weil 
> Signed-off-by: Thomas Huth 
> ---
>  configure  | 8 +++-
>  tests/tcg/configure.sh | 6 --
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 6099be1d84..fe6d0971f1 100755
> --- a/configure
> +++ b/configure
> @@ -302,6 +302,7 @@ audio_win_int=""
>  libs_qga=""
>  debug_info="yes"
>  stack_protector=""
> +use_containers="yes"
>  
>  if test -e "$source_path/.git"
>  then
> @@ -1539,6 +1540,10 @@ for opt do
>;;
>--disable-plugins) plugins="no"
>;;
> +  --enable-containers) use_containers="yes"
> +  ;;
> +  --disable-containers) use_containers="no"
> +  ;;
>*)
>echo "ERROR: unknown option $opt"
>echo "Try '$0 --help' for more information"
> @@ -1722,6 +1727,7 @@ Advanced options (experts only):
> track the maximum stack usage of stacks created 
> by qemu_alloc_stack
>--enable-plugins
> enable plugins via shared library loading
> +  --disable-containers don't use containers for cross-building
>  
>  Optional features, enabled with --enable-FEATURE and
>  disabled with --disable-FEATURE, default is enabled if available:
> @@ -8039,7 +8045,7 @@ done
>  (for i in $cross_cc_vars; do
>export $i
>  done
> -export target_list source_path
> +export target_list source_path use_containers
>  $source_path/tests/tcg/configure.sh)
>  
>  # temporary config to build submodules
> diff --git a/tests/tcg/configure.sh b/tests/tcg/configure.sh
> index 6c4a471aea..210e68396f 100755
> --- a/tests/tcg/configure.sh
> +++ b/tests/tcg/configure.sh
> @@ -36,8 +36,10 @@ TMPC="${TMPDIR1}/qemu-conf.c"
>  TMPE="${TMPDIR1}/qemu-conf.exe"
>  
>  container="no"
> -if has "docker" || has "podman"; then
> -  container=$($python $source_path/tests/docker/docker.py probe)
> +if test $use_containers = "yes"; then
> +if has "docker" || has "podman"; then
> +container=$($python $source_path/tests/docker/docker.py probe)
> +fi
>  fi
>  
>  # cross compilers defaults, can be overridden with --cross-cc-ARCH
> -- 
> 2.18.1
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 

Re: [PATCH v2 5/7] tests/test-util-filemonitor: Skip test on non-x86 Travis containers

[Cleber Rosa]

On Wed, Dec 04, 2019 at 04:46:16PM +0100, Thomas Huth wrote:
> test-util-filemonitor fails in restricted non-x86 Travis containers
> since they apparently blacklisted some required system calls there.
> Let's simply skip the test if we detect such an environment.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Alex Bennée 
> Signed-off-by: Thomas Huth 
> ---
>  tests/test-util-filemonitor.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/test-util-filemonitor.c b/tests/test-util-filemonitor.c
> index 301cd2db61..45009c69f4 100644
> --- a/tests/test-util-filemonitor.c
> +++ b/tests/test-util-filemonitor.c
> @@ -406,10 +406,21 @@ test_file_monitor_events(void)
>  char *pathdst = NULL;
>  QFileMonitorTestData data;
>  GHashTable *ids = g_hash_table_new(g_int64_hash, g_int64_equal);
> +char *travis_arch;
>  
>  qemu_mutex_init(&data.lock);
>  data.records = NULL;
>  
> +/*
> + * This test does not work on Travis LXD containers since some
> + * syscalls are blocked in that environment.
> + */
> +travis_arch = getenv("TRAVIS_ARCH");
> +if (travis_arch && !g_str_equal(travis_arch, "x86_64")) {
> +g_test_skip("Test does not work on non-x86 Travis containers.");
> +return;
> +}
> +
>      /*
>   * The file monitor needs the main loop running in
>   * order to receive events from inotify. We must
> -- 
> 2.18.1
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 

Re: [PATCH v2 4/7] tests/hd-geo-test: Skip test when images can not be created

[Cleber Rosa]

On Wed, Dec 04, 2019 at 04:46:15PM +0100, Thomas Huth wrote:
> In certain environments like restricted containers, we can not create
> huge test images. To be able to use "make check" in such container
> environments, too, let's skip the hd-geo-test instead of failing when
> the test images could not be created.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Alex Bennée 
> Signed-off-by: Thomas Huth 
> ---
>  tests/hd-geo-test.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 7e86c5416c..a249800544 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -34,8 +34,13 @@ static char *create_test_img(int secs)
>  fd = mkstemp(template);
>  g_assert(fd >= 0);
>  ret = ftruncate(fd, (off_t)secs * 512);
> -g_assert(ret == 0);
>  close(fd);
> +
> +if (ret) {
> +free(template);
> +template = NULL;
> +}
> +
>  return template;
>  }
>  
> @@ -934,6 +939,10 @@ int main(int argc, char **argv)
>  for (i = 0; i < backend_last; i++) {
>  if (img_secs[i] >= 0) {
>  img_file_name[i] = create_test_img(img_secs[i]);
> +if (!img_file_name[i]) {
> +g_test_message("Could not create test images.");
> +goto test_add_done;
> +}
>  } else {
>  img_file_name[i] = NULL;
>  }
> @@ -965,6 +974,7 @@ int main(int argc, char **argv)
> "skipping hd-geo/override/* tests");
>  }
>  
> +test_add_done:
>  ret = g_test_run();
>  
>  for (i = 0; i < backend_last; i++) {
> -- 
> 2.18.1
> 

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 

Re: [PATCH v2 2/7] iotests: Skip test 060 if it is not possible to create large files

[Cleber Rosa]

On Wed, Dec 04, 2019 at 04:46:13PM +0100, Thomas Huth wrote:
> Test 060 fails in the arm64, s390x and ppc64le LXD containers on Travis
> (which we will hopefully enable in our CI soon). These containers
> apparently do not allow large files to be created. The repair process
> in test 060 creates a file of 64 GiB, so test first whether such large
> files are possible and skip the test if that's not the case.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qemu-iotests/060 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index b91d8321bb..d96f17a484 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -49,6 +49,9 @@ _supported_fmt qcow2
>  _supported_proto file
>  _supported_os Linux
>  
> +# The repair process will create a large file - so check for availability 
> first
> +_require_large_file 64G
> +
>  rt_offset=65536  # 0x1 (XXX: just an assumption)
>  rb_offset=131072 # 0x2 (XXX: just an assumption)
>  l1_offset=196608 # 0x3 (XXX: just an assumption)
> -- 
> 2.18.1
> 

The behavior and failure is indeed pretty consistent accross those
architectures:

 - arm64: https://travis-ci.org/clebergnu/qemu/jobs/621238740#L4217
 - ppc64le: https://travis-ci.org/clebergnu/qemu/jobs/621238741#L4252
 - s390x: https://travis-ci.org/clebergnu/qemu/jobs/621238742#L4265

And with this, 060 gets skipped properly:

 - arm64: https://travis-ci.org/clebergnu/qemu/jobs/621248591#L4202
 - ppc64le: https://travis-ci.org/clebergnu/qemu/jobs/621248592#L4236
 - s390x: https://travis-ci.org/clebergnu/qemu/jobs/621248593#L4250

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 

Re: [PATCH v2 1/7] iotests: Provide a function for checking the creation of huge files

[Cleber Rosa]

On Wed, Dec 04, 2019 at 04:46:12PM +0100, Thomas Huth wrote:
> Some tests create huge (but sparse) files, and to be able to run those
> tests in certain limited environments (like CI containers), we have to
> check for the possibility to create such files first. Thus let's introduce
> a common function to check for large files, and replace the already
> existing checks in the iotests 005 and 220 with this function.
> 
> Reviewed-by: Alex Bennée 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qemu-iotests/005   |  5 +
>  tests/qemu-iotests/220   |  6 ++
>  tests/qemu-iotests/common.rc | 10 ++
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
> index 58442762fe..b6d03ac37d 100755
> --- a/tests/qemu-iotests/005
> +++ b/tests/qemu-iotests/005
> @@ -59,10 +59,7 @@ fi
>  # Sanity check: For raw, we require a file system that permits the creation
>  # of a HUGE (but very sparse) file. Check we can create it before continuing.
>  if [ "$IMGFMT" = "raw" ]; then
> -if ! truncate --size=5T "$TEST_IMG"; then
> -_notrun "file system on $TEST_DIR does not support large enough 
> files"
> -fi
> -rm "$TEST_IMG"
> +_require_large_file 5T
>  fi
>  
>  echo
> diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
> index 2d62c5dcac..15159270d3 100755
> --- a/tests/qemu-iotests/220
> +++ b/tests/qemu-iotests/220
> @@ -42,10 +42,8 @@ echo "== Creating huge file =="
>  
>  # Sanity check: We require a file system that permits the creation
>  # of a HUGE (but very sparse) file.  tmpfs works, ext4 does not.
> -if ! truncate --size=513T "$TEST_IMG"; then
> -_notrun "file system on $TEST_DIR does not support large enough files"
> -fi
> -rm "$TEST_IMG"
> +_require_large_file 513T
> +
>  IMGOPTS='cluster_size=2M,refcount_bits=1' _make_test_img 513T
>  
>  echo "== Populating refcounts =="
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 0cc8acc9ed..6f0582c79a 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -643,5 +643,15 @@ _require_drivers()
>  done
>  }
>  
> +# Check that we have a file system that allows huge (but very sparse) files
> +#
> +_require_large_file()
> +{
> +if ! truncate --size="$1" "$TEST_IMG"; then
> +_notrun "file system on $TEST_DIR does not support large enough 
> files"
> +fi
> +rm "$TEST_IMG"
> +}
> +
>  # make sure this script returns success
>  true
> -- 
> 2.18.1
> 

This is a good refactor even without considering the CI environment
issues it will help to address.

Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa