Libguestfs - Mar 2018 - Re: [PATCH] tests: regressions: make test-big-heap use a temporary empty file

Newer versions of qemu use file locking for the images by default, and
apparently that does not work with /dev/null.  Since this test just
calls qemu-img to get the format of an empty image, create a temporary
one instead.
---
 tests/regressions/test-big-heap.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tests/regressions/test-big-heap.c
b/tests/regressions/test-big-heap.c
index b841fba70..5c0948252 100644
--- a/tests/regressions/test-big-heap.c
+++ b/tests/regressions/test-big-heap.c
@@ -31,6 +31,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <assert.h>
+#include <unistd.h>
 
 #include "guestfs.h"
 #include "guestfs-utils.h"
@@ -41,6 +42,8 @@ main (int argc, char *argv[])
   const char *s;
   guestfs_h *g;
   char *mem, *fmt;
+  char tmpfile[32];
+  int tmpfilefd;
 
   /* Allow the test to be skipped. */
   s = getenv ("SKIP_TEST_BIG_HEAP");
@@ -50,6 +53,8 @@ main (int argc, char *argv[])
     exit (77);
   }
 
+  snprintf (tmpfile, sizeof tmpfile, "test-big-heap.XXXXXX");
+
   /* Make sure we're using > 1GB in the main process.  This test
won't
    * work on 32 bit platforms, because we can't allocate 2GB of
    * contiguous memory.  Therefore skip the test if the calloc call
@@ -68,10 +73,19 @@ main (int argc, char *argv[])
     exit (77);
   }
 
+  /* Create an empty temporary file for qemu-img. */
+  tmpfilefd = mkstemp (tmpfile);
+  if (tmpfilefd == -1) {
+    fprintf (stderr, "%s: mkstemp failed: %m\n", argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  close (tmpfilefd);
+
   g = guestfs_create ();
 
   /* Do something which forks qemu-img subprocess. */
-  fmt = guestfs_disk_format (g, "/dev/null");
+  fmt = guestfs_disk_format (g, tmpfile);
+  unlink (tmpfile);
   if (fmt == NULL) {
     /* Test failed. */
     fprintf (stderr, "%s: unexpected failure of test, see earlier
messages\n",
-- 
2.14.3

On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano
wrote:
> Newer versions of qemu use file locking for the images by default, and
> apparently that does not work with /dev/null.  Since this test just
> calls qemu-img to get the format of an empty image, create a temporary
> one instead.
ACK, but feels like this is a bug in qemu-img to me.

Rich.
>  tests/regressions/test-big-heap.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/regressions/test-big-heap.c
b/tests/regressions/test-big-heap.c
> index b841fba70..5c0948252 100644
> --- a/tests/regressions/test-big-heap.c
> +++ b/tests/regressions/test-big-heap.c
> @@ -31,6 +31,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <assert.h>
> +#include <unistd.h>
>  
>  #include "guestfs.h"
>  #include "guestfs-utils.h"
> @@ -41,6 +42,8 @@ main (int argc, char *argv[])
>    const char *s;
>    guestfs_h *g;
>    char *mem, *fmt;
> +  char tmpfile[32];
> +  int tmpfilefd;
>  
>    /* Allow the test to be skipped. */
>    s = getenv ("SKIP_TEST_BIG_HEAP");
> @@ -50,6 +53,8 @@ main (int argc, char *argv[])
>      exit (77);
>    }
>  
> +  snprintf (tmpfile, sizeof tmpfile, "test-big-heap.XXXXXX");
> +
>    /* Make sure we're using > 1GB in the main process.  This test
won't
>     * work on 32 bit platforms, because we can't allocate 2GB of
>     * contiguous memory.  Therefore skip the test if the calloc call
> @@ -68,10 +73,19 @@ main (int argc, char *argv[])
>      exit (77);
>    }
>  
> +  /* Create an empty temporary file for qemu-img. */
> +  tmpfilefd = mkstemp (tmpfile);
> +  if (tmpfilefd == -1) {
> +    fprintf (stderr, "%s: mkstemp failed: %m\n", argv[0]);
> +    exit (EXIT_FAILURE);
> +  }
> +  close (tmpfilefd);
> +
>    g = guestfs_create ();
>  
>    /* Do something which forks qemu-img subprocess. */
> -  fmt = guestfs_disk_format (g, "/dev/null");
> +  fmt = guestfs_disk_format (g, tmpfile);
> +  unlink (tmpfile);
>    if (fmt == NULL) {
>      /* Test failed. */
>      fprintf (stderr, "%s: unexpected failure of test, see earlier
messages\n",
> -- 
> 2.14.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

[adding qemu lists]

On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
>> Newer versions of qemu use file locking for the images by default, and
>> apparently that does not work with /dev/null.  Since this test just
>> calls qemu-img to get the format of an empty image, create a temporary
>> one instead.
> 
> ACK, but feels like this is a bug in qemu-img to me.
You're right that file locking on a character device like /dev/null is 
not going to work as expected, but is it a case where fcntl() actually 
fails, or is it worse where the fcntl() claiming the locks "succeeds" 
but doesn't do what we want?  That is, what were the actual error 
messages you ran into?

Is it something where qemu should instead be checking fstat() results 
and only doing locking on regular files and block devices?  At any rate, 
I agree that qemu itself should behave nicer on attempts to use 
/dev/null as an image.

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