Libguestfs - Aug 2009 - [PATCH] Don't use cache=off if device is on tmpfs

[For a bug noted earlier by both Matt Booth and Jim Meyering]

If you use the guestfs_add_drive function, then currently it
generates a qemu command line element like:

  -drive ...,cache=off,...

This causes qemu to try to open the device with O_DIRECT.
Unfortunately some filesystems don't support this flag, notably tmpfs,
which means you can't use libguestfs in conjunction with tmpfs.  On
some systems /tmp is a tmpfs filesystem.

This patch fixes this so that if the filesystem doesn't support
O_DIRECT, then we omit the cache=off parameter.  This seems reasonable
from a reliability point of view, because if you're using tmpfs then
you probably didn't expect reliability in the case where your system
suddenly powers off.

Tested using a tmpfs.  Without:

$ guestfish alloc /mnt/tmp/test.img 10M : run
qemu: could not open disk image /mnt/tmp/test.img
libguestfs: error: connect: Connection refused
libguestfs: error: connect: Connection refused
libguestfs: error: connect: Connection refused
libguestfs: error: connect: Connection refused

With:

$ ./fish/guestfish

Welcome to guestfish, the libguestfs filesystem interactive shell for
editing virtual machine filesystems.

Type: 'help' for help with commands
      'quit' to quit the shell
><fs> alloc /mnt/tmp/test.img 10M
><fs> run
><fs> 
Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-------------- next part --------------
>From 05e20ab5687fc629e5f7ec40aa252e01796191ea Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Wed, 12 Aug 2009 22:12:20 +0100
Subject: [PATCH] add_drive: Don't use cache=off if not supported by
underlying filesystem.

If you use the guestfs_add_drive function, then currently it
generates a qemu command line element like:

  -drive ...,cache=off,...

This causes qemu to try to open the device with O_DIRECT.
Unfortunately some filesystems don't support this flag, notably tmpfs,
which means you can't use libguestfs in conjunction with tmpfs.  On
some systems /tmp is a tmpfs filesystem.

This patch fixes this so that if the filesystem doesn't support
O_DIRECT, then we omit the cache=off parameter.  This seems reasonable
from a reliability point of view, because if you're using tmpfs then
you probably didn't expect reliability in the case where your system
suddenly powers off.
---
 src/generator.ml |    2 ++
 src/guestfs.c    |   30 ++++++++++++++++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/src/generator.ml b/src/generator.ml
index b9544ff..fa1f483 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -454,6 +454,8 @@ image).
 
 This is equivalent to the qemu parameter
 C<-drive file=filename,cache=off,if=...>.
+C<cache=off> is omitted in cases where it is not supported by
+the underlying filesystem.
 
 Note that this call checks for the existence of C<filename>.  This
 stops you from specifying other types of drive which are supported
diff --git a/src/guestfs.c b/src/guestfs.c
index 37869e8..7f309c9 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -793,20 +793,38 @@ guestfs_add_drive (guestfs_h *g, const char *filename)
 {
   size_t len = strlen (filename) + 64;
   char buf[len];
+  int fd;
 
   if (strchr (filename, ',') != NULL) {
     error (g, _("filename cannot contain ',' (comma)
character"));
     return -1;
   }
 
-  if (access (filename, F_OK) == -1) {
-    perrorf (g, "%s", filename);
-    return -1;
+  /* cache=off improves reliability in the event of a host crash.
+   *
+   * However this option causes qemu to try to open the file with
+   * O_DIRECT.  This fails on some filesystem types (notably tmpfs).
+   * So we check if we can open the file with or without O_DIRECT,
+   * and use cache=off (or not) accordingly.
+   *
+   * This test also checks for the presence of the file, which
+   * is a documented semantic of this interface.
+   */
+  fd = open (filename, O_RDONLY|O_DIRECT);
+  if (fd >= 0) {
+    close (fd);
+    snprintf (buf, len, "file=%s,cache=off,if=" DRIVE_IF, filename);
+  } else {
+    fd = open (filename, O_RDONLY);
+    if (fd >= 0) {
+      close (fd);
+      snprintf (buf, len, "file=%s,if=" DRIVE_IF, filename);
+    } else {
+      perrorf (g, "%s", filename);
+      return -1;
+    }
   }
 
-  /* cache=off improves reliability in the event of a host crash. */
-  snprintf (buf, len, "file=%s,cache=off,if=%s", filename, DRIVE_IF);
-
   return guestfs_config (g, "-drive", buf);
 }
 
-- 
1.6.2.5

On 12/08/09 22:14, Richard W.M. Jones wrote:
> diff --git a/src/generator.ml b/src/generator.ml
> index b9544ff..fa1f483 100755
> --- a/src/generator.ml
> +++ b/src/generator.ml
> @@ -454,6 +454,8 @@ image).
>
>   This is equivalent to the qemu parameter
>   C<-drive file=filename,cache=off,if=...>.
> +C<cache=off>  is omitted in cases where it is not supported by
> +the underlying filesystem.
>
>   Note that this call checks for the existence of C<filename>.  This
>   stops you from specifying other types of drive which are supported
> diff --git a/src/guestfs.c b/src/guestfs.c
> index 37869e8..7f309c9 100644
> --- a/src/guestfs.c
> +++ b/src/guestfs.c
> @@ -793,20 +793,38 @@ guestfs_add_drive (guestfs_h *g, const char
*filename)
>   {
>     size_t len = strlen (filename) + 64;
>     char buf[len];
> +  int fd;
>
I thought we'd decided to go with 'inline' declarations, whatever
the
appropriate term for those is.
>     if (strchr (filename, ',') != NULL) {
>       error (g, _("filename cannot contain ',' (comma)
character"));
>       return -1;
>     }
>
> -  if (access (filename, F_OK) == -1) {
> -    perrorf (g, "%s", filename);
> -    return -1;
> +  /* cache=off improves reliability in the event of a host crash.
> +   *
> +   * However this option causes qemu to try to open the file with
> +   * O_DIRECT.  This fails on some filesystem types (notably tmpfs).
> +   * So we check if we can open the file with or without O_DIRECT,
> +   * and use cache=off (or not) accordingly.
> +   *
> +   * This test also checks for the presence of the file, which
> +   * is a documented semantic of this interface.
> +   */
> +  fd = open (filename, O_RDONLY|O_DIRECT);
> +  if (fd>= 0) {
> +    close (fd);
> +    snprintf (buf, len, "file=%s,cache=off,if=" DRIVE_IF,
filename);
> +  } else {
> +    fd = open (filename, O_RDONLY);
> +    if (fd>= 0) {
> +      close (fd);
> +      snprintf (buf, len, "file=%s,if=" DRIVE_IF, filename);
> +    } else {
> +      perrorf (g, "%s", filename);
> +      return -1;
> +    }
>     }
>
> -  /* cache=off improves reliability in the event of a host crash. */
> -  snprintf (buf, len, "file=%s,cache=off,if=%s", filename,
DRIVE_IF);
> -
>     return guestfs_config (g, "-drive", buf);
>   }
>
> -- 1.6.2.5
ACK.

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

M:       +44 (0)7977 267231
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490

I pushed this one with the small change that Matt requested.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw