[Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

This is a follow up to a previous discussion about reported failures when
running some qemu-iotests.  Turns out the failures were due to missing
libraries, which in turn, reflected on the host build configuration.

This series introduces a tool that can check both host and target level
build configurations.  On top of that, it adds a function to to be used
on qemu-iotests.  Finally, as an example, it sets a test to be skipped
if the required feature is not enable on the host build configuration.

Cleber Rosa (3):
  scripts: introduce buildconf.py
  qemu-iotests: add _require_feature() function
  qemu-iotests: require CONFIG_LINUX_AIO for test 087

 scripts/buildconf.py         | 278 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/087       |   1 +
 tests/qemu-iotests/check     |   2 +
 tests/qemu-iotests/common.rc |   7 ++
 4 files changed, 288 insertions(+)

[Qemu-devel] [PATCH 1/3] scripts: introduce buildconf.py

[Cleber Rosa]

scripts/buildconf.py is a command line utility (but also can be used
as a Python module) that introspects the build configuration.

It uses the generated host level config-host.mak to obtain the general
build configuration, and optionally, also target specific
config-target.mak and config-devices.mak.  It does not attempt to
implement a Makefile parser, but instead relies on "make" itself to
parse those files and output the queried variable.

It requires a build tree that has been both configured and built.  By
default, for convenience, it will selected a default target, which can
be displayed and overriden.  A few examples follow.

To get the TLS priority (a host level configuration), one would run:

 $ ./scripts/buildconf.py CONFIG_TLS_PRIORITY
 NORMAL

To get a configuration from the default target devices:

 $ ./scripts/buildconf.py CONFIG_PARALLEL
 y

If one is not interested in the actual value, but whether a given
feature is enabled, the '-c|--check' option can be used:

 $ ./scripts/buildconf.py -c CONFIG_PARALLEL; echo $?
 0

And for checking a target different than the default one:

 $ ./scripts/buildconf.py -c CONFIG_PARALLEL arm-softmmu; echo $?
 255

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 scripts/buildconf.py | 278 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 278 insertions(+)
 create mode 100755 scripts/buildconf.py

diff --git a/scripts/buildconf.py b/scripts/buildconf.py
new file mode 100755
index 0000000..0eee6d7
--- /dev/null
+++ b/scripts/buildconf.py
@@ -0,0 +1,278 @@
+#!/usr/bin/env python
+#
+# QEMU build configuration introspection utilty
+#
+# Copyright (C) 2017 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+#
+# Authors:
+#  Cleber Rosa <crosa@redhat.com>
+
+"""
+QEMU's build configuration is recorded in both config-host.mak and
+config-host.h, as is specific target configuration expressed in
+<target>/config-{target,devices}.{mak,h}.
+
+This module relies on the .mak files, as they contain a bit more
+information than the .h files.
+
+While it would be possible to write a simple Makefile parser capable to
+handle the variable assignments, this would impose limitations on this
+script and introduce breakages if the build scripts start using
+functionality not expected here.
+
+The approach chosen was one that is definitely slower at runtime but
+is more reliable.  Temporary Makefiles that include config-host.mak,
+and optionally the target specific config-target.mak and
+config-devices.mak files, and print the desired configuration to
+stdout.  As long as the basic premises of a global config-host.mak,
+and target specific config-target.mak and config-devices.mak is kept,
+this tool should be able to keep up with any style or feature chances.
+"""
+
+from __future__ import print_function
+
+import optparse
+import os
+import subprocess
+import sys
+import tempfile
+
+
+TEMPLATE = """
+include {build_prefix}/config-host.mak
+{target_specific}
+
+all:
+	@echo $({conf})
+"""
+
+TARGET_TEMPLATE = """
+include {build_prefix}/{target}/config-target.mak
+include {build_prefix}/{target}/config-devices.mak
+"""
+
+
+class InvalidTarget(Exception):
+    """
+    Target chosen is not present in the current build tree configuration
+    """
+
+
+def get_build_root():
+    """
+    Returns the absolute location of the root of the build tree
+
+    This has been tested from build(only) trees, and works fine when
+    it's executed as a command line tool.
+
+    If this is used as a Python module, it will really depend on how
+    the module is imported.  If the build tree "scripts" directory is
+    added to the import path, this will work properly.  If the current
+    working directory is the "scripts" directory itself, and no
+    explicit import path is added, it will only work when building
+    from the source tree, and will *not* work when the build tree is
+    different from the source tree.
+
+    A longer explanation on this caveat: the Python import
+    implementation will look for a matching module in the current
+    working directory.  Python's current working directory,
+    os.getcwd(), is really like getcwd(3), and not like
+    os.getenv("PWD").  Because the scripts directory is linked to the
+    source tree, os.getcwd() returns the source tree location instead.
+    """
+    return os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+
+
+def is_build_root_configured():
+    """
+    Checks if the build root has been configured
+
+    In theory, this only makes sense for in-tree builds, because the
+    out-of-tree build directory, including the link to the scripts
+    directory containing this script, will only exist after a
+    successful "./configure" execution.
+
+    Either way, the check is still valid for the main source of
+    build configuration, that is, the existence of 'config-host.mak'
+    """
+    return os.path.isfile(os.path.join(get_build_root(), 'config-host.mak'))
+
+
+def get_default_target():
+    """
+    Returns the default target on the current build tree
+
+    The approach used here is to look for a "-softmmu" target that
+    matches "ARCH".  If found, that is the best choice for a default
+    target.
+
+    If not found, the first "-softmmu" target found is considered the
+    next best choice for a default target.
+
+    As a fallback if no "-softmmu" target exists, the first entry on
+    the target list is returned.
+
+    :returns: a target name or None if no target is configured
+    """
+    targets = get_targets()
+    if not targets:
+        return None
+    arch = get_build_conf("ARCH", None)
+    first_choice = "%s-softmmu" % arch
+    if first_choice in targets:
+        return first_choice
+    else:
+        softmmu_targets = [t for t in targets if t.endswith("-softmmu")]
+        if softmmu_targets:
+            return softmmu_targets[0]
+        else:
+            return targets[0]
+
+
+def get_build_conf(conf, target=None):
+    """
+    Returns the value of a given Makefile variable
+
+    :param conf: the configuration name, which really must be a
+                 Makefile variable in either the host or target .mak
+                 files
+    :param target: the name of a valid target in the current build tree.
+                   it must match the name of a target dir, such as
+                   'x86_64-softmmu' or 'i386-linux-user'.
+    :returns: the raw output or None
+    :rtype: str or None
+    """
+    build_prefix = get_build_root()
+
+    if target is None:
+        target_specific = ''
+    else:
+        if target not in get_targets():
+            raise InvalidTarget
+        target_specific = TARGET_TEMPLATE.format(build_prefix=build_prefix,
+                                                 target=target)
+
+    mak_fd, mak_path = tempfile.mkstemp()
+    os.write(mak_fd, TEMPLATE.format(build_prefix=build_prefix,
+                                     target_specific=target_specific,
+                                     conf=conf))
+    proc = subprocess.Popen(['make', '-f', mak_path],
+                            stdout=subprocess.PIPE,
+                            stderr=subprocess.PIPE)
+    ret = proc.wait()
+    os.unlink(mak_path)
+    if (ret == 0):
+        return proc.stdout.read().strip()
+
+
+def is_enabled(conf, target=None):
+    """
+    Checks wether a given feature is enabled in the build configuration
+
+    The Makefile variables default to using 'y' when they are enabled,
+    and are just missing (instead of set no 'n') when they're not enabled.
+    Even if a given variable is set, for instance in the case of TARGET_DIRS,
+    it will not be considered enabled by this function unless its content is
+    'y'.  For variables that are known to not contain 'y', please resort to
+    using get_build_conf() and parsing its output for meaningful value.
+
+    :param conf: the configuration name, which really must be a
+                 Makefile variable in either the host or target .mak
+                 files
+    :param target: the name of a valid target in the current build tree.
+                   it must match the name of a target dir, such as
+                   'x86_64-softmmu' or 'i386-linux-user'.
+    :returns: the raw output or None
+    :rtype: str or None
+    """
+    build_conf = get_build_conf(conf, target=target)
+    if build_conf == 'y':
+        return True
+    return False
+
+
+def get_targets():
+    """
+    Returns the list of targets currently configured
+
+    :rtype: list
+    """
+    targets = get_build_conf('TARGET_DIRS', None)
+    if targets is not None:
+        return targets.split()
+
+
+if __name__ == '__main__':
+    class Parser(optparse.OptionParser):
+
+        def __init__(self):
+            optparse.OptionParser.__init__(
+                self,
+                usage=('%prog [options] CONFIG [TARGET]\n\n'
+                       'CONFIG is the build configuration variable name\n'
+                       'TARGET is auto selected if not explicitly set'))
+            self.add_option('-c', '--check', action='store_true',
+                            help=('Checks if the build configuration option is '
+                                  'set to "y".  This causes this tool to be silent '
+                                  'and return only a status code of either 0 (if '
+                                  'configuration is set) or non-zero otherwise.'))
+            self.add_option('-n', '--no-default-target', action='store_true',
+                            help=('Do not attempt to use a default target if one '
+                                  'was not explicitly given in the command line'))
+            self.add_option('--print-target', action='store_true',
+                            help=('Also prints the selected target'))
+
+
+    class App(object):
+
+        def __init__(self):
+            self.target = None
+            self.parser = Parser()
+            self._parse()
+
+        def _parse(self):
+            self.opts, self.args = self.parser.parse_args()
+            args_len = len(self.args)
+            if (args_len < 1 or args_len > 2):
+                self.parser.print_help()
+                sys.exit(0)
+            elif args_len == 2:
+                self.target = self.args[1]
+            else:
+                if not self.opts.no_default_target:
+                    self.target = get_default_target()
+
+        def run(self):
+            if self.opts.print_target:
+                print("TARGET:", self.target)
+            config = self.args[0]
+            if self.opts.check:
+                result = is_enabled(config, self.target)
+                if result:
+                    sys.exit(0)
+                else:
+                    sys.exit(-1)
+            else:
+                conf = get_build_conf(config, self.target)
+                if conf:
+                    print(conf)
+                    sys.exit(0)
+                else:
+                    sys.exit(-1)
+
+
+    app = App()
+    app.run()
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/3] scripts: introduce buildconf.py

[Eric Blake]

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

On 07/20/2017 10:47 PM, Cleber Rosa wrote:
> scripts/buildconf.py is a command line utility (but also can be used
> as a Python module) that introspects the build configuration.
> 
> It uses the generated host level config-host.mak to obtain the general
> build configuration, and optionally, also target specific
> config-target.mak and config-devices.mak.  It does not attempt to
> implement a Makefile parser, but instead relies on "make" itself to
> parse those files and output the queried variable.
> 
> It requires a build tree that has been both configured and built.  By
> default, for convenience, it will selected a default target, which can
> be displayed and overriden.  A few examples follow.

s/overriden/overridden/


> 
> And for checking a target different than the default one:
> 
>  $ ./scripts/buildconf.py -c CONFIG_PARALLEL arm-softmmu; echo $?
>  255

Gross.  exit status greater than 128 typically mean death due to signal,
meanwhile, 255 is special-cased by find to kill processing immediately;
it is very rare that someone intentionally returns a status of 255, and
more often, it is evidence that someone mistakenly used exit(-1).

> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/buildconf.py | 278 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 278 insertions(+)
>  create mode 100755 scripts/buildconf.py
> 

I don't feel comfortable reviewing the script in depth, but I will request:


> +                else:
> +                    sys.exit(-1)
> +            else:
> +                conf = get_build_conf(config, self.target)
> +                if conf:
> +                    print(conf)
> +                    sys.exit(0)
> +                else:
> +                    sys.exit(-1)

Please use sys.exit(1), not -1.

Overall, the idea is cool.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/3] scripts: introduce buildconf.py

[Cleber Rosa]

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



On 07/21/2017 10:00 AM, Eric Blake wrote:
> On 07/20/2017 10:47 PM, Cleber Rosa wrote:
>> scripts/buildconf.py is a command line utility (but also can be used
>> as a Python module) that introspects the build configuration.
>>
>> It uses the generated host level config-host.mak to obtain the general
>> build configuration, and optionally, also target specific
>> config-target.mak and config-devices.mak.  It does not attempt to
>> implement a Makefile parser, but instead relies on "make" itself to
>> parse those files and output the queried variable.
>>
>> It requires a build tree that has been both configured and built.  By
>> default, for convenience, it will selected a default target, which can
>> be displayed and overriden.  A few examples follow.
> 
> s/overriden/overridden/
> 
> 

Oops, thanks for spotting it.

>>
>> And for checking a target different than the default one:
>>
>>  $ ./scripts/buildconf.py -c CONFIG_PARALLEL arm-softmmu; echo $?
>>  255
> 
> Gross.  exit status greater than 128 typically mean death due to signal,
> meanwhile, 255 is special-cased by find to kill processing immediately;
> it is very rare that someone intentionally returns a status of 255, and
> more often, it is evidence that someone mistakenly used exit(-1).
> 

Yuck! Yes, it was a clear mistake.

>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  scripts/buildconf.py | 278 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 278 insertions(+)
>>  create mode 100755 scripts/buildconf.py
>>
> 
> I don't feel comfortable reviewing the script in depth, but I will request:
> 
> 
>> +                else:
>> +                    sys.exit(-1)
>> +            else:
>> +                conf = get_build_conf(config, self.target)
>> +                if conf:
>> +                    print(conf)
>> +                    sys.exit(0)
>> +                else:
>> +                    sys.exit(-1)
> 
> Please use sys.exit(1), not -1.
> 

Sure, I 'll fix that.

> Overall, the idea is cool.
> 

Thanks.  I mean, there's one big issue about the general approach, which
is to require a build root.  Let's see how this specific problem gets a
solution.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/3] qemu-iotests: add _require_feature() function

[Cleber Rosa]

With the previously introduced buildconf.py script, it's possible
to determine if a feature needed by a test is present or not.  This
adds a thin layer on top of scripts/buildconf.py, and allows tests
to be skipped when a feature is required.

The naming of the function, while different in tense from the
_supported_* family of functions, was chosen to match the style
of _require_command(), which seems pretty similar.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/qemu-iotests/check     | 2 ++
 tests/qemu-iotests/common.rc | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2a55ec9..c0f4004 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -60,6 +60,8 @@ fi
 
 build_root="$build_iotests/../.."
 
+export BUILDCONF="$build_root/scripts/buildconf.py"
+
 if [ -x "$build_iotests/socket_scm_helper" ]
 then
     export SOCKET_SCM_HELPER="$build_iotests/socket_scm_helper"
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 2548e58..19b3111 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -470,6 +470,13 @@ _require_command()
     [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
 }
 
+# tests whether a given configure time feature is enabled
+#
+_require_feature()
+{
+    $BUILDCONF -c -n $1 || _notrun "feature not enabled: $1"
+}
+
 _full_imgfmt_details()
 {
     if [ -n "$IMGOPTS" ]; then
-- 
2.9.4

[Qemu-devel] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Cleber Rosa]

One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.

As a PoC/RFC, this goes the easy route and skips the test as a whole
when that feature is missing.  Other approaches include splitting
the test and adding extra filtering.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 tests/qemu-iotests/087 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index f8e4903..a2fb7de 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -34,6 +34,7 @@ status=1	# failure is the default!
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+_require_feature CONFIG_LINUX_AIO
 
 function do_run_qemu()
 {
-- 
2.9.4

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Jing Liu]

Hi Cleber,


On 2017/7/21 上午11:47, Cleber Rosa wrote:
> One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
>
> As a PoC/RFC, this goes the easy route and skips the test as a whole
> when that feature is missing.  Other approaches include splitting
> the test and adding extra filtering.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   tests/qemu-iotests/087 | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> index f8e4903..a2fb7de 100755
> --- a/tests/qemu-iotests/087
> +++ b/tests/qemu-iotests/087
> @@ -34,6 +34,7 @@ status=1	# failure is the default!
>   _supported_fmt qcow2
>   _supported_proto file
>   _supported_os Linux
> +_require_feature CONFIG_LINUX_AIO
I tested that CONFIG_NETTLE_KDF is also a necessary for 087.

+_require_feature CONFIG_NETTLE_KDF


>
>   function do_run_qemu()
>   {

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Stefan Hajnoczi]

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

On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
> On 2017/7/21 上午11:47, Cleber Rosa wrote:
> > One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
> > 
> > As a PoC/RFC, this goes the easy route and skips the test as a whole
> > when that feature is missing.  Other approaches include splitting
> > the test and adding extra filtering.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   tests/qemu-iotests/087 | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > index f8e4903..a2fb7de 100755
> > --- a/tests/qemu-iotests/087
> > +++ b/tests/qemu-iotests/087
> > @@ -34,6 +34,7 @@ status=1	# failure is the default!
> >   _supported_fmt qcow2
> >   _supported_proto file
> >   _supported_os Linux
> > +_require_feature CONFIG_LINUX_AIO
> I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
> 
> +_require_feature CONFIG_NETTLE_KDF

Are you sure?  Looks like either nettle or gcrypt is needed:

crypto/Makefile.objs:crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o
crypto/Makefile.objs:crypto-obj-$(if $(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o

But this shows why the compile-time testing of features is ugly:

1. It duplicates build dependency logic into the test cases.

2. The test cases don't care about nettle vs gcrypt and they shouldn't
   have to know about it.  They just care whether LUKS is available or
   not.

As I mentioned earlier, let's test whether the QEMU binaries offer
_features_, not which _config options_ they were built with.

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

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Daniel P. Berrange]

On Tue, Jul 25, 2017 at 04:45:46PM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
> > On 2017/7/21 上午11:47, Cleber Rosa wrote:
> > > One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
> > > 
> > > As a PoC/RFC, this goes the easy route and skips the test as a whole
> > > when that feature is missing.  Other approaches include splitting
> > > the test and adding extra filtering.
> > > 
> > > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > > ---
> > >   tests/qemu-iotests/087 | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > > index f8e4903..a2fb7de 100755
> > > --- a/tests/qemu-iotests/087
> > > +++ b/tests/qemu-iotests/087
> > > @@ -34,6 +34,7 @@ status=1	# failure is the default!
> > >   _supported_fmt qcow2
> > >   _supported_proto file
> > >   _supported_os Linux
> > > +_require_feature CONFIG_LINUX_AIO
> > I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
> > 
> > +_require_feature CONFIG_NETTLE_KDF
> 
> Are you sure?  Looks like either nettle or gcrypt is needed:

Correct, it works with either.

> crypto/Makefile.objs:crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o
> crypto/Makefile.objs:crypto-obj-$(if $(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o
> 
> But this shows why the compile-time testing of features is ugly:
> 
> 1. It duplicates build dependency logic into the test cases.
> 
> 2. The test cases don't care about nettle vs gcrypt and they shouldn't
>    have to know about it.  They just care whether LUKS is available or
>    not.

Yeah that knowledge is messy and fragile wrt future changes.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Jing Liu]

On 2017/7/25 下午11:48, Daniel P. Berrange wrote:
> On Tue, Jul 25, 2017 at 04:45:46PM +0100, Stefan Hajnoczi wrote:
>> On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
>>> On 2017/7/21 上午11:47, Cleber Rosa wrote:
>>>> One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
>>>>
>>>> As a PoC/RFC, this goes the easy route and skips the test as a whole
>>>> when that feature is missing.  Other approaches include splitting
>>>> the test and adding extra filtering.
>>>>
>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/087 | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
>>>> index f8e4903..a2fb7de 100755
>>>> --- a/tests/qemu-iotests/087
>>>> +++ b/tests/qemu-iotests/087
>>>> @@ -34,6 +34,7 @@ status=1	# failure is the default!
>>>>    _supported_fmt qcow2
>>>>    _supported_proto file
>>>>    _supported_os Linux
>>>> +_require_feature CONFIG_LINUX_AIO
>>> I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
>>>
>>> +_require_feature CONFIG_NETTLE_KDF
>> Are you sure?  Looks like either nettle or gcrypt is needed:
> Correct, it works with either.
Ah, because I just found out nettle which related to KDF.
why can not find out gcrypt.h in qemu?
How to compile config_gcrypt_kdf into qemu?
>
>> crypto/Makefile.objs:crypto-obj-$(CONFIG_NETTLE_KDF) += pbkdf-nettle.o
>> crypto/Makefile.objs:crypto-obj-$(if $(CONFIG_NETTLE_KDF),n,$(CONFIG_GCRYPT_KDF)) += pbkdf-gcrypt.o
>>
>> But this shows why the compile-time testing of features is ugly:
>>
>> 1. It duplicates build dependency logic into the test cases.
>>
>> 2. The test cases don't care about nettle vs gcrypt and they shouldn't
>>     have to know about it.  They just care whether LUKS is available or
>>     not.
> Yeah that knowledge is messy and fragile wrt future changes.
>
>
> Regards,
> Daniel

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] qemu-iotests: require CONFIG_LINUX_AIO for test 087

[Daniel P. Berrange]

On Wed, Jul 26, 2017 at 04:55:39PM +0800, Jing Liu wrote:
> 
> 
> On 2017/7/25 下午11:48, Daniel P. Berrange wrote:
> > On Tue, Jul 25, 2017 at 04:45:46PM +0100, Stefan Hajnoczi wrote:
> > > On Mon, Jul 24, 2017 at 02:44:13PM +0800, Jing Liu wrote:
> > > > On 2017/7/21 上午11:47, Cleber Rosa wrote:
> > > > > One of the "sub-"tests of test 087 requires CONFIG_LINUX_AIO.
> > > > > 
> > > > > As a PoC/RFC, this goes the easy route and skips the test as a whole
> > > > > when that feature is missing.  Other approaches include splitting
> > > > > the test and adding extra filtering.
> > > > > 
> > > > > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > > > > ---
> > > > >    tests/qemu-iotests/087 | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> > > > > index f8e4903..a2fb7de 100755
> > > > > --- a/tests/qemu-iotests/087
> > > > > +++ b/tests/qemu-iotests/087
> > > > > @@ -34,6 +34,7 @@ status=1	# failure is the default!
> > > > >    _supported_fmt qcow2
> > > > >    _supported_proto file
> > > > >    _supported_os Linux
> > > > > +_require_feature CONFIG_LINUX_AIO
> > > > I tested that CONFIG_NETTLE_KDF is also a necessary for 087.
> > > > 
> > > > +_require_feature CONFIG_NETTLE_KDF
> > > Are you sure?  Looks like either nettle or gcrypt is needed:
> > Correct, it works with either.
> Ah, because I just found out nettle which related to KDF.
> why can not find out gcrypt.h in qemu?
> How to compile config_gcrypt_kdf into qemu?

QEMU picks either nettle or libgcrypt automatically, dependant on which
of these gnutls is linked to.

You can force override this via  --disable-nettle --enable-gcrypt
(or vica-verca), for sake of testing, but you shouldn't do that
for production builds.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip
Message-id: 20170721034730.25612-1-crosa@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20170721041952.45950-1-aik@ozlabs.ru -> patchew/20170721041952.45950-1-aik@ozlabs.ru
Switched to a new branch 'test'
8e1715d qemu-iotests: require CONFIG_LINUX_AIO for test 087
2ba2d91 qemu-iotests: add _require_feature() function
9041490 scripts: introduce buildconf.py

=== OUTPUT BEGIN ===
Checking PATCH 1/3: scripts: introduce buildconf.py...
ERROR: code indent should never use tabs
#107: FILE: scripts/buildconf.py:59:
+^I@echo $({conf})$

WARNING: line over 80 characters
#277: FILE: scripts/buildconf.py:229:
+                                  'set to "y".  This causes this tool to be silent '

WARNING: line over 80 characters
#278: FILE: scripts/buildconf.py:230:
+                                  'and return only a status code of either 0 (if '

WARNING: line over 80 characters
#279: FILE: scripts/buildconf.py:231:
+                                  'configuration is set) or non-zero otherwise.'))

WARNING: line over 80 characters
#281: FILE: scripts/buildconf.py:233:
+                            help=('Do not attempt to use a default target if one '

WARNING: line over 80 characters
#282: FILE: scripts/buildconf.py:234:
+                                  'was not explicitly given in the command line'))

total: 1 errors, 5 warnings, 278 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/3: qemu-iotests: add _require_feature() function...
Checking PATCH 3/3: qemu-iotests: require CONFIG_LINUX_AIO for test 087...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

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

On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> This is a follow up to a previous discussion about reported failures when
> running some qemu-iotests.  Turns out the failures were due to missing
> libraries, which in turn, reflected on the host build configuration.
> 
> This series introduces a tool that can check both host and target level
> build configurations.  On top of that, it adds a function to to be used
> on qemu-iotests.  Finally, as an example, it sets a test to be skipped
> if the required feature is not enable on the host build configuration.
> 
> Cleber Rosa (3):
>   scripts: introduce buildconf.py
>   qemu-iotests: add _require_feature() function
>   qemu-iotests: require CONFIG_LINUX_AIO for test 087
> 
>  scripts/buildconf.py         | 278 +++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/087       |   1 +
>  tests/qemu-iotests/check     |   2 +
>  tests/qemu-iotests/common.rc |   7 ++
>  4 files changed, 288 insertions(+)
> 

It should be possible to run iotests against any
qemu/qemu-img/qemu-io/qemu-nbd binaries - even if no build root is
available.

How about invoking qemu-img and tools to determine their capabilities?

At the beginning of ./check, query the qemu/qemu-img/qemu-io/qemu-nbd
binaries for specific features.  This produces a set of available
features and tests can say:

  _supported_feature aio_native

This feature can be checked by opening an image file:

  qemu-io --format raw --nocache --native-aio --cmd quit test.img

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

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

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



On 07/21/2017 08:33 AM, Stefan Hajnoczi wrote:
> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>> This is a follow up to a previous discussion about reported failures when
>> running some qemu-iotests.  Turns out the failures were due to missing
>> libraries, which in turn, reflected on the host build configuration.
>>
>> This series introduces a tool that can check both host and target level
>> build configurations.  On top of that, it adds a function to to be used
>> on qemu-iotests.  Finally, as an example, it sets a test to be skipped
>> if the required feature is not enable on the host build configuration.
>>
>> Cleber Rosa (3):
>>   scripts: introduce buildconf.py
>>   qemu-iotests: add _require_feature() function
>>   qemu-iotests: require CONFIG_LINUX_AIO for test 087
>>
>>  scripts/buildconf.py         | 278 +++++++++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/087       |   1 +
>>  tests/qemu-iotests/check     |   2 +
>>  tests/qemu-iotests/common.rc |   7 ++
>>  4 files changed, 288 insertions(+)
>>
> 
> It should be possible to run iotests against any
> qemu/qemu-img/qemu-io/qemu-nbd binaries - even if no build root is
> available.
> 

Yes, I actually overlooked that point.

> How about invoking qemu-img and tools to determine their capabilities?
> 

Can capabilities be consistently queried?  I would love to not count on
a build root if the same information can be consistently queried from
the binaries themselves.

> At the beginning of ./check, query the qemu/qemu-img/qemu-io/qemu-nbd
> binaries for specific features.  This produces a set of available
> features and tests can say:
> 

Which would be another ad-hoc thing, limited to qemu-iotests.  From a
test writer perspective, QEMU lacks is a uniform way to introspect its
capabilities.

>   _supported_feature aio_native
> 
> This feature can be checked by opening an image file:
> 
>   qemu-io --format raw --nocache --native-aio --cmd quit test.img
> 

While the solution I proposed is not cheap in terms of what it runs to
query capabilities (runs make on every query), it was cheap to write, it
sets a universal standard, and it's mostly maintenance free.  A key
point is that all build configuration (capabilities?) is predictable and
available across all subsystems and all targets.

Being honest, I think your suggestion is terribly expensive in the long
run.  In the best case scenario, it requires one explicit check to be
written for each capability, which at some point, may start to look like
a test itself.  The capability naming and behavior will probably end up
becoming inconsistent.

I feel a lot more safe relying on a "capability statement" to write the
foundation of tests, than to write a number of custom "capability checks".

But I agree that the build root requirement is an issue.

Is embedding the configured capabilities in the binary themselves
acceptable?  Something like an standard option such as
`-query-capabilities` or `-debug-build-info` that would basically list
the content of "config-host.h" and similar files?

Thanks for reviewing the idea and pointing out this important limitation!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Markus Armbruster]

Cleber Rosa <crosa@redhat.com> writes:

> On 07/21/2017 08:33 AM, Stefan Hajnoczi wrote:
>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>>> This is a follow up to a previous discussion about reported failures when
>>> running some qemu-iotests.  Turns out the failures were due to missing
>>> libraries, which in turn, reflected on the host build configuration.
>>>
>>> This series introduces a tool that can check both host and target level
>>> build configurations.  On top of that, it adds a function to to be used
>>> on qemu-iotests.  Finally, as an example, it sets a test to be skipped
>>> if the required feature is not enable on the host build configuration.
>>>
>>> Cleber Rosa (3):
>>>   scripts: introduce buildconf.py
>>>   qemu-iotests: add _require_feature() function
>>>   qemu-iotests: require CONFIG_LINUX_AIO for test 087
>>>
>>>  scripts/buildconf.py         | 278 +++++++++++++++++++++++++++++++++++++++++++
>>>  tests/qemu-iotests/087       |   1 +
>>>  tests/qemu-iotests/check     |   2 +
>>>  tests/qemu-iotests/common.rc |   7 ++
>>>  4 files changed, 288 insertions(+)
>>>
>> 
>> It should be possible to run iotests against any
>> qemu/qemu-img/qemu-io/qemu-nbd binaries - even if no build root is
>> available.
>> 
>
> Yes, I actually overlooked that point.
>
>> How about invoking qemu-img and tools to determine their capabilities?
>> 
>
> Can capabilities be consistently queried?  I would love to not count on
> a build root if the same information can be consistently queried from
> the binaries themselves.
>
>> At the beginning of ./check, query the qemu/qemu-img/qemu-io/qemu-nbd
>> binaries for specific features.  This produces a set of available
>> features and tests can say:
>> 
>
> Which would be another ad-hoc thing, limited to qemu-iotests.  From a
> test writer perspective, QEMU lacks is a uniform way to introspect its
> capabilities.

The closest we have is query-qmp-schema.  It's uniform, but limited to
the qapified part of QMP (see my KVM Forum 2015 talk[*] for details).
Something similar for the command line would be nice, and I hope to get
there some day.  Until then, you can often reason like "if QMP supports
X, then surely the command line supports X'".

We commonly reason "if INTERFACE supports FEATURE-API, then QEMU surely
supports FEATURE".

[...]

[*] https://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Daniel P. Berrange]

On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> > This is a follow up to a previous discussion about reported failures when
> > running some qemu-iotests.  Turns out the failures were due to missing
> > libraries, which in turn, reflected on the host build configuration.
> > 
> > This series introduces a tool that can check both host and target level
> > build configurations.  On top of that, it adds a function to to be used
> > on qemu-iotests.  Finally, as an example, it sets a test to be skipped
> > if the required feature is not enable on the host build configuration.
> > 
> > Cleber Rosa (3):
> >   scripts: introduce buildconf.py
> >   qemu-iotests: add _require_feature() function
> >   qemu-iotests: require CONFIG_LINUX_AIO for test 087
> > 
> >  scripts/buildconf.py         | 278 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/087       |   1 +
> >  tests/qemu-iotests/check     |   2 +
> >  tests/qemu-iotests/common.rc |   7 ++
> >  4 files changed, 288 insertions(+)
> > 
> 
> It should be possible to run iotests against any
> qemu/qemu-img/qemu-io/qemu-nbd binaries - even if no build root is
> available.

For sake of argument, two options for non-buildroot scenario

 - assume all features are present, so we're no worse than we are today.
 - install config.h (or same data in a structured format) to
   /usr/share/qemu so its available for query

Downside of 2 of course is that other non-iotests apps might start
to depend on it

> How about invoking qemu-img and tools to determine their capabilities?
> 
> At the beginning of ./check, query the qemu/qemu-img/qemu-io/qemu-nbd
> binaries for specific features.  This produces a set of available
> features and tests can say:
> 
>   _supported_feature aio_native
> 
> This feature can be checked by opening an image file:
> 
>   qemu-io --format raw --nocache --native-aio --cmd quit test.img

I think this is useful as a general approach, because there are bound
to be a number of features which are available at compile time, but
cannot actually be used at runtime. eg not every filesystem supports
O_DIRECT, even if we've built support for it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

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



On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>>> This is a follow up to a previous discussion about reported failures when
>>> running some qemu-iotests.  Turns out the failures were due to missing
>>> libraries, which in turn, reflected on the host build configuration.
>>>
>>> This series introduces a tool that can check both host and target level
>>> build configurations.  On top of that, it adds a function to to be used
>>> on qemu-iotests.  Finally, as an example, it sets a test to be skipped
>>> if the required feature is not enable on the host build configuration.
>>>
>>> Cleber Rosa (3):
>>>   scripts: introduce buildconf.py
>>>   qemu-iotests: add _require_feature() function
>>>   qemu-iotests: require CONFIG_LINUX_AIO for test 087
>>>
>>>  scripts/buildconf.py         | 278 +++++++++++++++++++++++++++++++++++++++++++
>>>  tests/qemu-iotests/087       |   1 +
>>>  tests/qemu-iotests/check     |   2 +
>>>  tests/qemu-iotests/common.rc |   7 ++
>>>  4 files changed, 288 insertions(+)
>>>
>>
>> It should be possible to run iotests against any
>> qemu/qemu-img/qemu-io/qemu-nbd binaries - even if no build root is
>> available.
> 
> For sake of argument, two options for non-buildroot scenario
> 
>  - assume all features are present, so we're no worse than we are today.
>  - install config.h (or same data in a structured format) to
>    /usr/share/qemu so its available for query
> 
> Downside of 2 of course is that other non-iotests apps might start
> to depend on it
> 

Actually, I see #2 as a worthy goal.  Not in the strict sense of the
implementation you suggested, but as a way for *any code* (including
non-iotests) to have a baseline to work with.

>> How about invoking qemu-img and tools to determine their capabilities?
>>
>> At the beginning of ./check, query the qemu/qemu-img/qemu-io/qemu-nbd
>> binaries for specific features.  This produces a set of available
>> features and tests can say:
>>
>>   _supported_feature aio_native
>>
>> This feature can be checked by opening an image file:
>>
>>   qemu-io --format raw --nocache --native-aio --cmd quit test.img
> 
> I think this is useful as a general approach, because there are bound
> to be a number of features which are available at compile time, but
> cannot actually be used at runtime. eg not every filesystem supports
> O_DIRECT, even if we've built support for it.
> 

I strongly believe this kind of ad hoc check has value, it complements
what is being proposed here, but doesn't replace it at all.  Using the
previous example, suppose a test is being written to test aio in various
filesystems.  It'd be extremely useful to rely on the information that
qemu-io itself has been built with native aio support.  With that
information as a safe baseline, and run time information about the
filesystem it's operating on, a much cleaner expected outcome can be
defined.

Without the static capabilities defined, the dynamic check would be
influenced by the run time environment.  It would really mean "qemu-io
running on this environment (filesystem?) can do native aio".  Again,
that's not the best type of information to depend on when writing tests.

> Regards,
> Daniel
> 

Regards!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

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

On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> > On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> Without the static capabilities defined, the dynamic check would be
> influenced by the run time environment.  It would really mean "qemu-io
> running on this environment (filesystem?) can do native aio".  Again,
> that's not the best type of information to depend on when writing tests.

Can you explain this more?

It seems logical to me that if qemu-io in this environment cannot do
aio=native then we must skip those tests.

Stefan

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

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

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



On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>> Without the static capabilities defined, the dynamic check would be
>> influenced by the run time environment.  It would really mean "qemu-io
>> running on this environment (filesystem?) can do native aio".  Again,
>> that's not the best type of information to depend on when writing tests.
> 
> Can you explain this more?
> 
> It seems logical to me that if qemu-io in this environment cannot do
> aio=native then we must skip those tests.
> 
> Stefan
> 

OK, let's abstract a bit more.  Let's take this part of your statement:

 "if qemu-io in this environment cannot do aio=native"

Let's call that a feature check.  Depending on how the *feature check*
is written, a negative result may hide a test failure, because it would
now be skipped.

Suppose that a feature check for "SDL display" is such that you run
"qemu -display sdl".  A *feature failure* here (SDL init is broken), or
an environment issue (DISPLAY=), will cause a SDL test skip.

If you base the test skip decision to a simple lookup on a list of
features (not calling them build configuration anymore, as this is clear
not attractive), this won't happen.  A "feature statement check" will
make the test proceed, and the *failure* will be presented.

I hope the pattern is visible.

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Daniel P. Berrange]

On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> 
> 
> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> > On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >> Without the static capabilities defined, the dynamic check would be
> >> influenced by the run time environment.  It would really mean "qemu-io
> >> running on this environment (filesystem?) can do native aio".  Again,
> >> that's not the best type of information to depend on when writing tests.
> > 
> > Can you explain this more?
> > 
> > It seems logical to me that if qemu-io in this environment cannot do
> > aio=native then we must skip those tests.
> > 
> > Stefan
> > 
> 
> OK, let's abstract a bit more.  Let's take this part of your statement:
> 
>  "if qemu-io in this environment cannot do aio=native"
> 
> Let's call that a feature check.  Depending on how the *feature check*
> is written, a negative result may hide a test failure, because it would
> now be skipped.
> 
> Suppose that a feature check for "SDL display" is such that you run
> "qemu -display sdl".  A *feature failure* here (SDL init is broken), or
> an environment issue (DISPLAY=), will cause a SDL test skip.

You could have a way to statically define what features any given run
of the test suite should enable, then report failure if they were not
detected.

This is a similar situation to that seen with configure scripts. If invoked
with no --enable-xxx flags, it will probe for features & enable them if
found.  This means you can accidentally build without expected features if
you have a missing -devel package, or a header/library is broken in some
way. This is why configure prints a summary of which features it actually
found. It is also why when building binary packages like RPMs, it is common
to explicitly give --enable-xxx flags for all features you expect to see.
Automatic enablement is none the less still useful for people in general.

So if we applied this kind of approach for testing, then any automated
test systems at least, ought to provide a fixed list of features they
expect to be present for tests. So if any features accidentally broke
the tests would error.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

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



On 07/25/2017 12:24 PM, Daniel P. Berrange wrote:
>>
>> OK, let's abstract a bit more.  Let's take this part of your statement:
>>
>>  "if qemu-io in this environment cannot do aio=native"
>>
>> Let's call that a feature check.  Depending on how the *feature check*
>> is written, a negative result may hide a test failure, because it would
>> now be skipped.
>>
>> Suppose that a feature check for "SDL display" is such that you run
>> "qemu -display sdl".  A *feature failure* here (SDL init is broken), or
>> an environment issue (DISPLAY=), will cause a SDL test skip.
> 
> You could have a way to statically define what features any given run
> of the test suite should enable, then report failure if they were not
> detected.
> 

You hit a key point here: statically define(d).  As a said before,
feature statements are a safer place upon which to base tests.  Ad hoc
checks, as suggested by Stefan, are definitely not.

> This is a similar situation to that seen with configure scripts. If invoked
> with no --enable-xxx flags, it will probe for features & enable them if
> found.  This means you can accidentally build without expected features if
> you have a missing -devel package, or a header/library is broken in some
> way. This is why configure prints a summary of which features it actually
> found. It is also why when building binary packages like RPMs, it is common
> to explicitly give --enable-xxx flags for all features you expect to see.
> Automatic enablement is none the less still useful for people in general.
> 
> So if we applied this kind of approach for testing, then any automated
> test systems at least, ought to provide a fixed list of features they
> expect to be present for tests. So if any features accidentally broke
> the tests would error.
> 
> Regards,
> Daniel
> 

Right.  The key question here seems to be the distance of the "fixed
list of features" from the test itself.  For instance, think of this
workflow/approach:

 1) ./scripts/configured-features-to-feature-list.sh > ~/feature_list
 2) tweak feature_list
 3) ./rpm -e SDL-devel
 4) ./configure  --enable-sdl
 5) ./make
 6) ./scripts/run-test-suite.sh --only-features=~/feature_list

This would only run tests that are expected to PASS within the given
feature list.  The test runner (run-test-suite.sh) would select only
tests that match the features given.  No SKIPs would be expected as the
outcome of *any test*.

The other approach is to let the feature match to the test, and SKIPs
would then be OK.  The downside to this is that a "--enable-xxx" with
missing "-devel" package, as you exemplified, would not show up as ERRORs.

Makes sense?

Regards!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

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

On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> > On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >> Without the static capabilities defined, the dynamic check would be
> >> influenced by the run time environment.  It would really mean "qemu-io
> >> running on this environment (filesystem?) can do native aio".  Again,
> >> that's not the best type of information to depend on when writing tests.
> > 
> > Can you explain this more?
> > 
> > It seems logical to me that if qemu-io in this environment cannot do
> > aio=native then we must skip those tests.
> > 
> > Stefan
> > 
> 
> OK, let's abstract a bit more.  Let's take this part of your statement:
> 
>  "if qemu-io in this environment cannot do aio=native"
> 
> Let's call that a feature check.  Depending on how the *feature check*
> is written, a negative result may hide a test failure, because it would
> now be skipped.

You are saying a pass->skip transition can hide a failure but ./check
tracks skipped tests.  See tests/qemu-iotests/check.log for a
pass/fail/skip history.

It is the job of the CI system to flag up pass/fail/skip transitions.
You're no worse off using feature tests.

Stefan

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

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Cleber Rosa]

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



On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
> On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
>> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
>>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>>>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>>>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>>>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>>>> Without the static capabilities defined, the dynamic check would be
>>>> influenced by the run time environment.  It would really mean "qemu-io
>>>> running on this environment (filesystem?) can do native aio".  Again,
>>>> that's not the best type of information to depend on when writing tests.
>>>
>>> Can you explain this more?
>>>
>>> It seems logical to me that if qemu-io in this environment cannot do
>>> aio=native then we must skip those tests.
>>>
>>> Stefan
>>>
>>
>> OK, let's abstract a bit more.  Let's take this part of your statement:
>>
>>  "if qemu-io in this environment cannot do aio=native"
>>
>> Let's call that a feature check.  Depending on how the *feature check*
>> is written, a negative result may hide a test failure, because it would
>> now be skipped.
> 
> You are saying a pass->skip transition can hide a failure but ./check
> tracks skipped tests.  See tests/qemu-iotests/check.log for a
> pass/fail/skip history.
> 

You're not focusing on the problem here.  The problem is that a test
that *was not* supposed to be skipped, would be skipped.

Let me reinforce my point, and you can address it directly:  feature
checks like you proposed can easily produce false negatives.  Not
something hypothetical or far fetched, I gave very reasonable examples
on this same thread.

Do you think that's OK because the skip count will get an increment?
That's exactly one of the main concerns raised in the original thread (
and break room conversations) that motivated this experiment.

> It is the job of the CI system to flag up pass/fail/skip transitions.
> You're no worse off using feature tests.
> 
> Stefan
> 

What I'm trying to help us achieve here is a reliable and predictable
way for the same test job execution to be comparable across
environments.  From the individual developer workstation, CI, QA etc.

Please let me know If you really believe this should *not* be done here
(upstream QEMU).

Regards!

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

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

On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
> 
> 
> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >>>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >>>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >>>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >>>> Without the static capabilities defined, the dynamic check would be
> >>>> influenced by the run time environment.  It would really mean "qemu-io
> >>>> running on this environment (filesystem?) can do native aio".  Again,
> >>>> that's not the best type of information to depend on when writing tests.
> >>>
> >>> Can you explain this more?
> >>>
> >>> It seems logical to me that if qemu-io in this environment cannot do
> >>> aio=native then we must skip those tests.
> >>>
> >>> Stefan
> >>>
> >>
> >> OK, let's abstract a bit more.  Let's take this part of your statement:
> >>
> >>  "if qemu-io in this environment cannot do aio=native"
> >>
> >> Let's call that a feature check.  Depending on how the *feature check*
> >> is written, a negative result may hide a test failure, because it would
> >> now be skipped.
> > 
> > You are saying a pass->skip transition can hide a failure but ./check
> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
> > pass/fail/skip history.
> > 
> 
> You're not focusing on the problem here.  The problem is that a test
> that *was not* supposed to be skipped, would be skipped.

As Daniel Berrange mentioned, ./configure has the same problem.  You
cannot just run it blindly because it silently disables features.

What I'm saying is that in addition to watching ./configure closely, you
also need to look at the skipped tests that ./check reports.  If you do
that then you can be sure the expected set of tests is passing.

> > It is the job of the CI system to flag up pass/fail/skip transitions.
> > You're no worse off using feature tests.
> > 
> > Stefan
> > 
> 
> What I'm trying to help us achieve here is a reliable and predictable
> way for the same test job execution to be comparable across
> environments.  From the individual developer workstation, CI, QA etc.

1. Use ./configure --enable-foo options for all desired features.
2. Run the ./check command-line and there should be no unexpected skips
   like this:

087         [not run] missing aio=native support

To me this seems to address the problem.

I have mentioned the issues with the build flags solution: it creates a
dependency on the build environment and forces test feature checks to
duplicate build dependency logic.  This is why I think feature tests are
a cleaner solution.

Stefan

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

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
>> 
>> 
>> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
>> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
>> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
>> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>> >>>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>> >>>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>> >>>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>> >>>> Without the static capabilities defined, the dynamic check would be
>> >>>> influenced by the run time environment.  It would really mean "qemu-io
>> >>>> running on this environment (filesystem?) can do native aio".  Again,
>> >>>> that's not the best type of information to depend on when writing tests.
>> >>>
>> >>> Can you explain this more?
>> >>>
>> >>> It seems logical to me that if qemu-io in this environment cannot do
>> >>> aio=native then we must skip those tests.
>> >>>
>> >>> Stefan
>> >>>
>> >>
>> >> OK, let's abstract a bit more.  Let's take this part of your statement:
>> >>
>> >>  "if qemu-io in this environment cannot do aio=native"
>> >>
>> >> Let's call that a feature check.  Depending on how the *feature check*
>> >> is written, a negative result may hide a test failure, because it would
>> >> now be skipped.
>> > 
>> > You are saying a pass->skip transition can hide a failure but ./check
>> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
>> > pass/fail/skip history.
>> > 
>> 
>> You're not focusing on the problem here.  The problem is that a test
>> that *was not* supposed to be skipped, would be skipped.
>
> As Daniel Berrange mentioned, ./configure has the same problem.  You
> cannot just run it blindly because it silently disables features.
>
> What I'm saying is that in addition to watching ./configure closely, you
> also need to look at the skipped tests that ./check reports.  If you do
> that then you can be sure the expected set of tests is passing.
>
>> > It is the job of the CI system to flag up pass/fail/skip transitions.
>> > You're no worse off using feature tests.
>> > 
>> > Stefan
>> > 
>> 
>> What I'm trying to help us achieve here is a reliable and predictable
>> way for the same test job execution to be comparable across
>> environments.  From the individual developer workstation, CI, QA etc.
>
> 1. Use ./configure --enable-foo options for all desired features.
> 2. Run the ./check command-line and there should be no unexpected skips
>    like this:
>
> 087         [not run] missing aio=native support
>
> To me this seems to address the problem.
>
> I have mentioned the issues with the build flags solution: it creates a
> dependency on the build environment and forces test feature checks to
> duplicate build dependency logic.  This is why I think feature tests are
> a cleaner solution.

I suspect the actual problem here is that the qemu-iotests harness is
not integrated in the build process.  For other tests, we specify the
tests to run in a Makefile, and use the same configuration mechanism as
for building stuff conditionally.

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

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

On Tue, Aug 08, 2017 at 10:06:04AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
> >> 
> >> 
> >> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
> >> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> >> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> >> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >> >>>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >> >>>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >> >>>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >> >>>> Without the static capabilities defined, the dynamic check would be
> >> >>>> influenced by the run time environment.  It would really mean "qemu-io
> >> >>>> running on this environment (filesystem?) can do native aio".  Again,
> >> >>>> that's not the best type of information to depend on when writing tests.
> >> >>>
> >> >>> Can you explain this more?
> >> >>>
> >> >>> It seems logical to me that if qemu-io in this environment cannot do
> >> >>> aio=native then we must skip those tests.
> >> >>>
> >> >>> Stefan
> >> >>>
> >> >>
> >> >> OK, let's abstract a bit more.  Let's take this part of your statement:
> >> >>
> >> >>  "if qemu-io in this environment cannot do aio=native"
> >> >>
> >> >> Let's call that a feature check.  Depending on how the *feature check*
> >> >> is written, a negative result may hide a test failure, because it would
> >> >> now be skipped.
> >> > 
> >> > You are saying a pass->skip transition can hide a failure but ./check
> >> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
> >> > pass/fail/skip history.
> >> > 
> >> 
> >> You're not focusing on the problem here.  The problem is that a test
> >> that *was not* supposed to be skipped, would be skipped.
> >
> > As Daniel Berrange mentioned, ./configure has the same problem.  You
> > cannot just run it blindly because it silently disables features.
> >
> > What I'm saying is that in addition to watching ./configure closely, you
> > also need to look at the skipped tests that ./check reports.  If you do
> > that then you can be sure the expected set of tests is passing.
> >
> >> > It is the job of the CI system to flag up pass/fail/skip transitions.
> >> > You're no worse off using feature tests.
> >> > 
> >> > Stefan
> >> > 
> >> 
> >> What I'm trying to help us achieve here is a reliable and predictable
> >> way for the same test job execution to be comparable across
> >> environments.  From the individual developer workstation, CI, QA etc.
> >
> > 1. Use ./configure --enable-foo options for all desired features.
> > 2. Run the ./check command-line and there should be no unexpected skips
> >    like this:
> >
> > 087         [not run] missing aio=native support
> >
> > To me this seems to address the problem.
> >
> > I have mentioned the issues with the build flags solution: it creates a
> > dependency on the build environment and forces test feature checks to
> > duplicate build dependency logic.  This is why I think feature tests are
> > a cleaner solution.
> 
> I suspect the actual problem here is that the qemu-iotests harness is
> not integrated in the build process.  For other tests, we specify the
> tests to run in a Makefile, and use the same configuration mechanism as
> for building stuff conditionally.

The ability to run tests against QEMU binaries without a build
environment is useful though.  It would still be possible to symlink to
external binaries but then the build feature information could be
incorrect.

Stefan

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

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Tue, Aug 08, 2017 at 10:06:04AM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>> 
>> > On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
>> >> 
>> >> 
>> >> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
>> >> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
>> >> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
>> >> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
>> >> >>>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
>> >> >>>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
>> >> >>>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
>> >> >>>> Without the static capabilities defined, the dynamic check would be
>> >> >>>> influenced by the run time environment.  It would really mean "qemu-io
>> >> >>>> running on this environment (filesystem?) can do native aio".  Again,
>> >> >>>> that's not the best type of information to depend on when writing tests.
>> >> >>>
>> >> >>> Can you explain this more?
>> >> >>>
>> >> >>> It seems logical to me that if qemu-io in this environment cannot do
>> >> >>> aio=native then we must skip those tests.
>> >> >>>
>> >> >>> Stefan
>> >> >>>
>> >> >>
>> >> >> OK, let's abstract a bit more.  Let's take this part of your statement:
>> >> >>
>> >> >>  "if qemu-io in this environment cannot do aio=native"
>> >> >>
>> >> >> Let's call that a feature check.  Depending on how the *feature check*
>> >> >> is written, a negative result may hide a test failure, because it would
>> >> >> now be skipped.
>> >> > 
>> >> > You are saying a pass->skip transition can hide a failure but ./check
>> >> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
>> >> > pass/fail/skip history.
>> >> > 
>> >> 
>> >> You're not focusing on the problem here.  The problem is that a test
>> >> that *was not* supposed to be skipped, would be skipped.
>> >
>> > As Daniel Berrange mentioned, ./configure has the same problem.  You
>> > cannot just run it blindly because it silently disables features.
>> >
>> > What I'm saying is that in addition to watching ./configure closely, you
>> > also need to look at the skipped tests that ./check reports.  If you do
>> > that then you can be sure the expected set of tests is passing.
>> >
>> >> > It is the job of the CI system to flag up pass/fail/skip transitions.
>> >> > You're no worse off using feature tests.
>> >> > 
>> >> > Stefan
>> >> > 
>> >> 
>> >> What I'm trying to help us achieve here is a reliable and predictable
>> >> way for the same test job execution to be comparable across
>> >> environments.  From the individual developer workstation, CI, QA etc.
>> >
>> > 1. Use ./configure --enable-foo options for all desired features.
>> > 2. Run the ./check command-line and there should be no unexpected skips
>> >    like this:
>> >
>> > 087         [not run] missing aio=native support
>> >
>> > To me this seems to address the problem.
>> >
>> > I have mentioned the issues with the build flags solution: it creates a
>> > dependency on the build environment and forces test feature checks to
>> > duplicate build dependency logic.  This is why I think feature tests are
>> > a cleaner solution.
>> 
>> I suspect the actual problem here is that the qemu-iotests harness is
>> not integrated in the build process.  For other tests, we specify the
>> tests to run in a Makefile, and use the same configuration mechanism as
>> for building stuff conditionally.
>
> The ability to run tests against QEMU binaries without a build
> environment is useful though.  It would still be possible to symlink to
> external binaries but then the build feature information could be
> incorrect.

I don't dispute it's useful.  "make check" doesn't do it, though.

I think we can either have a standalone test suite (introspects the
binaries under test to figure out what to test), or an integrated test
suite (tests exactly what is configured).  "make check" is the latter.
qemu-iotests is kind-of-sort-of the former.

Re: [Qemu-devel] [PATCH 0/3] build configuration query tool and conditional (qemu-io)test skip

[Stefan Hajnoczi]

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

On Tue, Aug 08, 2017 at 04:52:25PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Tue, Aug 08, 2017 at 10:06:04AM +0200, Markus Armbruster wrote:
> >> Stefan Hajnoczi <stefanha@gmail.com> writes:
> >> 
> >> > On Wed, Jul 26, 2017 at 02:24:02PM -0400, Cleber Rosa wrote:
> >> >> 
> >> >> 
> >> >> On 07/26/2017 01:58 PM, Stefan Hajnoczi wrote:
> >> >> > On Tue, Jul 25, 2017 at 12:16:13PM -0400, Cleber Rosa wrote:
> >> >> >> On 07/25/2017 11:49 AM, Stefan Hajnoczi wrote:
> >> >> >>> On Fri, Jul 21, 2017 at 10:21:24AM -0400, Cleber Rosa wrote:
> >> >> >>>> On 07/21/2017 10:01 AM, Daniel P. Berrange wrote:
> >> >> >>>>> On Fri, Jul 21, 2017 at 01:33:25PM +0100, Stefan Hajnoczi wrote:
> >> >> >>>>>> On Thu, Jul 20, 2017 at 11:47:27PM -0400, Cleber Rosa wrote:
> >> >> >>>> Without the static capabilities defined, the dynamic check would be
> >> >> >>>> influenced by the run time environment.  It would really mean "qemu-io
> >> >> >>>> running on this environment (filesystem?) can do native aio".  Again,
> >> >> >>>> that's not the best type of information to depend on when writing tests.
> >> >> >>>
> >> >> >>> Can you explain this more?
> >> >> >>>
> >> >> >>> It seems logical to me that if qemu-io in this environment cannot do
> >> >> >>> aio=native then we must skip those tests.
> >> >> >>>
> >> >> >>> Stefan
> >> >> >>>
> >> >> >>
> >> >> >> OK, let's abstract a bit more.  Let's take this part of your statement:
> >> >> >>
> >> >> >>  "if qemu-io in this environment cannot do aio=native"
> >> >> >>
> >> >> >> Let's call that a feature check.  Depending on how the *feature check*
> >> >> >> is written, a negative result may hide a test failure, because it would
> >> >> >> now be skipped.
> >> >> > 
> >> >> > You are saying a pass->skip transition can hide a failure but ./check
> >> >> > tracks skipped tests.  See tests/qemu-iotests/check.log for a
> >> >> > pass/fail/skip history.
> >> >> > 
> >> >> 
> >> >> You're not focusing on the problem here.  The problem is that a test
> >> >> that *was not* supposed to be skipped, would be skipped.
> >> >
> >> > As Daniel Berrange mentioned, ./configure has the same problem.  You
> >> > cannot just run it blindly because it silently disables features.
> >> >
> >> > What I'm saying is that in addition to watching ./configure closely, you
> >> > also need to look at the skipped tests that ./check reports.  If you do
> >> > that then you can be sure the expected set of tests is passing.
> >> >
> >> >> > It is the job of the CI system to flag up pass/fail/skip transitions.
> >> >> > You're no worse off using feature tests.
> >> >> > 
> >> >> > Stefan
> >> >> > 
> >> >> 
> >> >> What I'm trying to help us achieve here is a reliable and predictable
> >> >> way for the same test job execution to be comparable across
> >> >> environments.  From the individual developer workstation, CI, QA etc.
> >> >
> >> > 1. Use ./configure --enable-foo options for all desired features.
> >> > 2. Run the ./check command-line and there should be no unexpected skips
> >> >    like this:
> >> >
> >> > 087         [not run] missing aio=native support
> >> >
> >> > To me this seems to address the problem.
> >> >
> >> > I have mentioned the issues with the build flags solution: it creates a
> >> > dependency on the build environment and forces test feature checks to
> >> > duplicate build dependency logic.  This is why I think feature tests are
> >> > a cleaner solution.
> >> 
> >> I suspect the actual problem here is that the qemu-iotests harness is
> >> not integrated in the build process.  For other tests, we specify the
> >> tests to run in a Makefile, and use the same configuration mechanism as
> >> for building stuff conditionally.
> >
> > The ability to run tests against QEMU binaries without a build
> > environment is useful though.  It would still be possible to symlink to
> > external binaries but then the build feature information could be
> > incorrect.
> 
> I don't dispute it's useful.  "make check" doesn't do it, though.
> 
> I think we can either have a standalone test suite (introspects the
> binaries under test to figure out what to test), or an integrated test
> suite (tests exactly what is configured).  "make check" is the latter.
> qemu-iotests is kind-of-sort-of the former.

Yes, originally qemu-iotests was a separate repo.  It was moved into
qemu.git so that it's easier to include tests in a patch series.  But as
a result of this history it has the ability to run against any QEMU.

Actually I'm not sure how important that ability is anymore.  Some
testing teams use qemu-iotests against QEMU binaries from elsewhere, so
we'd inconvenience them by tying it to a build.  But they could update
their process to get the QEMU tree that matches their binaries, if
necessary.

Stefan

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