[PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'

Peter Krempa posted 35 patches 1 year, 10 months ago
[PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'
Posted by Peter Krempa 1 year, 10 months ago
Test both situations (reading from non-regular file and reading a file
larger than (arbitrary) buffer size) via 'virshtest'.

To feed the pipe we need to create a thread that does it, but otherwise
it's fairly straightforward.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 tests/meson.build                     |  2 -
 tests/virsh-read-bufsiz               | 49 ---------------
 tests/virsh-read-non-seekable         | 51 ---------------
 tests/virshtest.c                     | 90 +++++++++++++++++++++++++++
 tests/virshtestdata/read-big-pipe.out |  7 +++
 5 files changed, 97 insertions(+), 102 deletions(-)
 delete mode 100755 tests/virsh-read-bufsiz
 delete mode 100755 tests/virsh-read-non-seekable
 create mode 100644 tests/virshtestdata/read-big-pipe.out

diff --git a/tests/meson.build b/tests/meson.build
index 46e1679ad7..c760fe68d9 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -691,8 +691,6 @@ if conf.has('WITH_LIBVIRTD')
     'libvirtd-fail',
     'libvirtd-pool',
     'virsh-auth',
-    'virsh-read-bufsiz',
-    'virsh-read-non-seekable',
     'virsh-self-test',
     'virsh-uriprecedence',
     'virt-admin-self-test',
diff --git a/tests/virsh-read-bufsiz b/tests/virsh-read-bufsiz
deleted file mode 100755
index a61816c25b..0000000000
--- a/tests/virsh-read-bufsiz
+++ /dev/null
@@ -1,49 +0,0 @@
-#!/bin/sh
-# ensure that reading a file larger than BUFSIZ works
-
-# Copyright (C) 2008, 2010 Red Hat, Inc.
-
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 2 of the License, or
-# (at your option) any later version.
-
-# This program 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 General Public License for more details.
-
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see
-# <http://www.gnu.org/licenses/>.
-
-. "$(dirname $0)/test-lib.sh"
-
-if test "$VERBOSE" = yes; then
-  set -x
-  $abs_top_builddir/tools/virsh --version
-fi
-
-fail=0
-
-# Output a valid definition, to be used as input.
-$abs_top_builddir/tools/virsh -c test:///default dumpxml 1 > xml.t || fail=1
-
-# Change the VM name and UUID
-sed -e "s|<name>test</name>|<name>newtest</name>|g" \
-  -e "\|<uuid>.*</uuid>|d" \
-  xml.t > xml
-
-for i in before after; do
-  # The largest BUFSIZ I've seen is 128K.  This is slightly larger.
-  printf %132000s ' ' > sp || fail=1
-  in=in-$i
-  # Append or prepend enough spaces to push the size over the limit:
-  ( test $i = before && cat sp xml || cat xml sp ) > $in || fail=1
-
-  $abs_top_builddir/tools/virsh --connect test:///default define $in > out || fail=1
-  printf "Domain 'newtest' defined from $in\n\n" > exp || fail=1
-  compare exp out || fail=1
-done
-
-(exit $fail); exit $fail
diff --git a/tests/virsh-read-non-seekable b/tests/virsh-read-non-seekable
deleted file mode 100755
index 0f7504c800..0000000000
--- a/tests/virsh-read-non-seekable
+++ /dev/null
@@ -1,51 +0,0 @@
-#!/bin/sh
-# ensure that certain file-reading commands can handle non-seekable files
-
-# Copyright (C) 2008 Red Hat, Inc.
-
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 2 of the License, or
-# (at your option) any later version.
-
-# This program 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 General Public License for more details.
-
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see
-# <http://www.gnu.org/licenses/>.
-
-. "$(dirname $0)/test-lib.sh"
-
-if test "$VERBOSE" = yes; then
-  set -x
-  $abs_top_builddir/tools/virsh --version
-fi
-
-fail=0
-
-cat <<\EOF > dom
-<domain type='test' id='2'>
-  <name>t2</name>
-  <uuid>004b96e1-2d78-c30f-5aa5-000000000000</uuid>
-  <memory>8388608</memory>
-  <vcpu>2</vcpu>
-  <os>
-    <type>xen</type>
-  </os>
-  <on_reboot>restart</on_reboot>
-  <on_poweroff>destroy</on_poweroff>
-  <on_crash>restart</on_crash>
-</domain>
-EOF
-
-$abs_top_builddir/tools/virsh -c test:///default define dom > /dev/null || fail=1
-
-mkfifo_or_skip_ fifo
-cat dom > fifo &
-
-$abs_top_builddir/tools/virsh -c test:///default define fifo > /dev/null || fail=1
-
-(exit $fail); exit $fail
diff --git a/tests/virshtest.c b/tests/virshtest.c
index 8bb94b7693..f520ff9fc9 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -1,11 +1,14 @@
 #include <config.h>

 #include <unistd.h>
+#include <fcntl.h>

 #include "internal.h"
 #include "testutils.h"
 #include "vircommand.h"

+#include "util/virthread.h"
+
 #define VIR_FROM_THIS VIR_FROM_NONE

 #ifdef WIN32
@@ -103,6 +106,90 @@ static int testCompare(const void *data)
 }


+static void
+testPipeFeeder(void *opaque)
+{
+    /* feed more than observed buffer size which was historically 128k in the
+     * test this was adapted from */
+    size_t emptyspace = 140 * 1024;
+    const char *pipepath = opaque;
+    const char *xml =
+        "<domain type='test' id='2'>\n"
+        "  <name>t2</name>\n"
+        "  <uuid>004b96e1-2d78-c30f-5aa5-000000000000</uuid>\n"
+        "  <memory>8388608</memory>\n"
+        "  <vcpu>2</vcpu>\n"
+        "  <os>\n"
+        "    <type>xen</type>\n"
+        "  </os>\n"
+        "</domain>\n";
+    size_t xmlsize = strlen(xml);
+    g_autofree char *doc = g_new0(char, emptyspace + xmlsize + 1);
+    VIR_AUTOCLOSE fd = -1;
+
+    if ((fd = open(pipepath, O_RDWR)) < 0) {
+        fprintf(stderr, "\nfailed to open pipe '%s': %s\n", pipepath, g_strerror(errno));
+        return;
+    }
+
+    memset(doc, ' ', emptyspace);
+    virStrcpy(doc + emptyspace, xml, xmlsize);
+
+    if (safewrite(fd, doc, emptyspace + xmlsize + 1) < 0) {
+        fprintf(stderr, "\nfailed to write to pipe '%s': %s\n", pipepath, g_strerror(errno));
+        return;
+    }
+}
+
+
+static int
+testVirshPipe(const void *data G_GNUC_UNUSED)
+{
+    char tmpdir[] = "/tmp/libvirt_virshtest_XXXXXXX";
+    g_autofree char *pipepath = NULL;
+    virThread feeder;
+    bool join = false;
+    g_autofree char *cmdstr = NULL;
+    const char *argv[] = { VIRSH_DEFAULT, NULL, NULL };
+    int ret = -1;
+
+    if (!g_mkdtemp(tmpdir)) {
+        fprintf(stderr, "\nfailed to create temporary directory\n");
+        return -1;
+    }
+
+    pipepath = g_strdup_printf("%s/pipe", tmpdir);
+
+
+    cmdstr = g_strdup_printf("define %s ; list --all", pipepath);
+    argv[3] = cmdstr;
+
+    if (mkfifo(pipepath, 0600) < 0) {
+        fprintf(stderr, "\nfailed to create pipe '%s': %s\n", pipepath, g_strerror(errno));
+        goto cleanup;
+    }
+
+    if (virThreadCreate(&feeder, true, testPipeFeeder, pipepath) < 0)
+        goto cleanup;
+
+    join = true;
+
+    if (testCompareOutputLit(abs_srcdir "/virshtestdata/read-big-pipe.out",
+                             "/tmp/libvirt_virshtest", argv) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    if (join)
+        virThreadJoin(&feeder);
+    unlink(pipepath);
+    rmdir(tmpdir);
+
+    return ret;
+}
+
+
 static int
 mymain(void)
 {
@@ -238,6 +325,9 @@ mymain(void)
                  "checkpoint-create test --redefine checkpoint-c2.xml ;"
                  "checkpoint-info test c2");

+    if (virTestRun("read-big-pipe", testVirshPipe, NULL) < 0)
+        ret = -1;
+
     VIR_FREE(custom_uri);
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
diff --git a/tests/virshtestdata/read-big-pipe.out b/tests/virshtestdata/read-big-pipe.out
new file mode 100644
index 0000000000..340432210d
--- /dev/null
+++ b/tests/virshtestdata/read-big-pipe.out
@@ -0,0 +1,7 @@
+Domain 't2' defined from 
+
+ Id   Name   State
+-----------------------
+ 1    test   running
+ -    t2     shut off
+
-- 
2.44.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'
Posted by Andrea Bolognani 1 year, 9 months ago
On Fri, Mar 22, 2024 at 06:56:08PM GMT, Peter Krempa wrote:
> +static void
> +testPipeFeeder(void *opaque)
> +{
> +    /* feed more than observed buffer size which was historically 128k in the
> +     * test this was adapted from */
> +    size_t emptyspace = 140 * 1024;

This test seems to fail consistently at least on ppc64le, among other
less common architectures. This can be seen both in Debian[1] and
Fedora[2]. It runs for a while, then it hits the timeout gets
terminated by meson.

I've reproduced it locally and this is the output:

  # LIBVIRT_DEBUG=1 VIR_TEST_DEBUG=1 VIR_TEST_RANGE=71 ./tests/virshtest
  ninja: no work to do.
  TEST: virshtest
  71) read-big-pipe
 ... 2024-05-07 16:43:17.099+0000: 69735: info : libvirt version:
10.4.0
  2024-05-07 16:43:17.099+0000: 69735: debug : virThreadJobSet:96 :
Thread 69735 is now running job testPipeFeeder
  2024-05-07 16:43:17.099+0000: 69734: debug : virCommandRunAsync:2657
: About to run LANG=C /root/libvirt/build/tools/virsh --connect
test:///default 'define /tmp/libvirt_virshtest_XUTXGN2/pipe ; list
--all'
  2024-05-07 16:43:17.099+0000: 69735: debug : virThreadJobClear:121 :
Thread 69735 finished job testPipeFeeder with ret=0
  2024-05-07 16:43:17.099+0000: 69734: debug : virCommandRunAsync:2659
: Command result 0, with PID 69736

I've bumped the size of emptyspace to 1024*1024 and that causes the
test to pass. 1023*1024 doesn't. Could it be something about the
fifo's capacity being different across architectures?


[1] https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=ppc64el&ver=10.3.0-2&stamp=1715074703&raw=0
[2] https://koji.fedoraproject.org/koji/taskinfo?taskID=117156020
-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'
Posted by Daniel P. Berrangé 1 year, 9 months ago
On Tue, May 07, 2024 at 04:56:00PM +0000, Andrea Bolognani wrote:
> On Fri, Mar 22, 2024 at 06:56:08PM GMT, Peter Krempa wrote:
> > +static void
> > +testPipeFeeder(void *opaque)
> > +{
> > +    /* feed more than observed buffer size which was historically 128k in the
> > +     * test this was adapted from */
> > +    size_t emptyspace = 140 * 1024;
> 
> This test seems to fail consistently at least on ppc64le, among other
> less common architectures. This can be seen both in Debian[1] and
> Fedora[2]. It runs for a while, then it hits the timeout gets
> terminated by meson.
> 
> I've reproduced it locally and this is the output:
> 
>   # LIBVIRT_DEBUG=1 VIR_TEST_DEBUG=1 VIR_TEST_RANGE=71 ./tests/virshtest
>   ninja: no work to do.
>   TEST: virshtest
>   71) read-big-pipe
>  ... 2024-05-07 16:43:17.099+0000: 69735: info : libvirt version:
> 10.4.0
>   2024-05-07 16:43:17.099+0000: 69735: debug : virThreadJobSet:96 :
> Thread 69735 is now running job testPipeFeeder
>   2024-05-07 16:43:17.099+0000: 69734: debug : virCommandRunAsync:2657
> : About to run LANG=C /root/libvirt/build/tools/virsh --connect
> test:///default 'define /tmp/libvirt_virshtest_XUTXGN2/pipe ; list
> --all'
>   2024-05-07 16:43:17.099+0000: 69735: debug : virThreadJobClear:121 :
> Thread 69735 finished job testPipeFeeder with ret=0
>   2024-05-07 16:43:17.099+0000: 69734: debug : virCommandRunAsync:2659
> : Command result 0, with PID 69736
> 
> I've bumped the size of emptyspace to 1024*1024 and that causes the
> test to pass. 1023*1024 doesn't. Could it be something about the
> fifo's capacity being different across architectures?

I had multiple builds fail in Fedora, but today a ppc64
build magically passed. So even on ppc64 it is racy :-(

The virFileReadAll method reads in BUFSIZ chunks but that
hasn't changed in years. The new test is pretty trivial,
and I struggle to see why changing the buffer size would
affect it, given the old test this replaced did largely
the same thing.

With 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 :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH 30/35] virshtest: Adapt 'virsh-read-bufsiz' and 'virsh-read-non-seekable'
Posted by Andrea Bolognani 1 year, 9 months ago
On Tue, May 07, 2024 at 08:14:18PM GMT, Daniel P. Berrangé wrote:
> On Tue, May 07, 2024 at 04:56:00PM +0000, Andrea Bolognani wrote:
> > On Fri, Mar 22, 2024 at 06:56:08PM GMT, Peter Krempa wrote:
> > > +static void
> > > +testPipeFeeder(void *opaque)
> > > +{
> > > +    /* feed more than observed buffer size which was historically 128k in the
> > > +     * test this was adapted from */
> > > +    size_t emptyspace = 140 * 1024;
> >
> > This test seems to fail consistently at least on ppc64le, among other
> > less common architectures. This can be seen both in Debian[1] and
> > Fedora[2]. It runs for a while, then it hits the timeout gets
> > terminated by meson.
> >
> > I've reproduced it locally and this is the output:
> >
> >   # LIBVIRT_DEBUG=1 VIR_TEST_DEBUG=1 VIR_TEST_RANGE=71 ./tests/virshtest
> >   ninja: no work to do.
> >   TEST: virshtest
> >   71) read-big-pipe
> >  ... 2024-05-07 16:43:17.099+0000: 69735: info : libvirt version:
> > 10.4.0
> >   2024-05-07 16:43:17.099+0000: 69735: debug : virThreadJobSet:96 :
> > Thread 69735 is now running job testPipeFeeder
> >   2024-05-07 16:43:17.099+0000: 69734: debug : virCommandRunAsync:2657
> > : About to run LANG=C /root/libvirt/build/tools/virsh --connect
> > test:///default 'define /tmp/libvirt_virshtest_XUTXGN2/pipe ; list
> > --all'
> >   2024-05-07 16:43:17.099+0000: 69735: debug : virThreadJobClear:121 :
> > Thread 69735 finished job testPipeFeeder with ret=0
> >   2024-05-07 16:43:17.099+0000: 69734: debug : virCommandRunAsync:2659
> > : Command result 0, with PID 69736
> >
> > I've bumped the size of emptyspace to 1024*1024 and that causes the
> > test to pass. 1023*1024 doesn't. Could it be something about the
> > fifo's capacity being different across architectures?
>
> I had multiple builds fail in Fedora, but today a ppc64
> build magically passed. So even on ppc64 it is racy :-(
>
> The virFileReadAll method reads in BUFSIZ chunks but that
> hasn't changed in years. The new test is pretty trivial,
> and I struggle to see why changing the buffer size would
> affect it, given the old test this replaced did largely
> the same thing.

Maybe the shell writes to the fifo differently than libvirt does?

Also "read XML from a fifo" and "read a really big XML file" were two
separate test cases before. Some unexpected interaction between
buffer size and fifo handling, perhaps?

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org