Libguestfs - Nov 2014 - [PATCH 2/3] New API: guestfs_blockdev_setra: Adjust readahead for filesystems and devices.

See http://rwmj.wordpress.com/2014/11/23/mapping-files-to-disk/

Commit 588af1953e5f7ab74009b9175cc5d3efb8bb651a started with a very
conservative list of permitted backing formats (just "raw" or
"qcow2").  We can allow almost any format permitted by qemu, but this
commit just adds "vmdk" to this whitelist.
---
 src/create.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/create.c b/src/create.c
index 621ace5..bfb8b76 100644
--- a/src/create.c
+++ b/src/create.c
@@ -261,7 +261,12 @@ disk_create_qcow2 (guestfs_h *g, const char *orig_filename,
int64_t size,
 
   if (optargs->bitmask & GUESTFS_DISK_CREATE_BACKINGFORMAT_BITMASK) {
     backingformat = optargs->backingformat;
-    if (STRNEQ (backingformat, "raw") && STRNEQ
(backingformat, "qcow2")) {
+    /* Conservative whitelist.  This can be extended with other
+     * valid formats as required.
+     */
+    if (STRNEQ (backingformat, "raw") &&
+        STRNEQ (backingformat, "qcow2") &&
+        STRNEQ (backingformat, "vmdk")) {
       error (g, _("invalid value for backingformat parameter
'%s'"),
              backingformat);
       return -1;
-- 
2.1.0

This adds a binding for 'blockdev --setra', allowing you to adjust the
readahead parameter for filesystems and devices.
---
 daemon/blockdev.c    | 30 ++++++++++++++++++++----------
 generator/actions.ml | 14 ++++++++++++++
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/daemon/blockdev.c b/daemon/blockdev.c
index 8a7b1a8..6e8821d 100644
--- a/daemon/blockdev.c
+++ b/daemon/blockdev.c
@@ -62,7 +62,7 @@ call_blockdev (const char *device, const char *switc, int
extraarg, int prints)
    */
   udev_settle ();
 
-  if (extraarg > 0) {
+  if (extraarg >= 0) {
     snprintf (buf, sizeof buf, "%d", extraarg);
     argv[2] = buf;
     argv[3] = device;
@@ -89,33 +89,43 @@ call_blockdev (const char *device, const char *switc, int
extraarg, int prints)
 }
 
 int
+do_blockdev_setra (const char *device, int sectors)
+{
+  if (sectors < 0) {
+    reply_with_error ("readahead sectors must be >= 0");
+    return -1;
+  }
+  return (int) call_blockdev (device, "--setra", sectors, 0);
+}
+
+int
 do_blockdev_setro (const char *device)
 {
-  return (int) call_blockdev (device, "--setro", 0, 0);
+  return (int) call_blockdev (device, "--setro", -1, 0);
 }
 
 int
 do_blockdev_setrw (const char *device)
 {
-  return (int) call_blockdev (device, "--setrw", 0, 0);
+  return (int) call_blockdev (device, "--setrw", -1, 0);
 }
 
 int
 do_blockdev_getro (const char *device)
 {
-  return (int) call_blockdev (device, "--getro", 0, 1);
+  return (int) call_blockdev (device, "--getro", -1, 1);
 }
 
 int
 do_blockdev_getss (const char *device)
 {
-  return (int) call_blockdev (device, "--getss", 0, 1);
+  return (int) call_blockdev (device, "--getss", -1, 1);
 }
 
 int
 do_blockdev_getbsz (const char *device)
 {
-  return (int) call_blockdev (device, "--getbsz", 0, 1);
+  return (int) call_blockdev (device, "--getbsz", -1, 1);
 }
 
 int
@@ -128,23 +138,23 @@ do_blockdev_setbsz (const char *device, int blocksize)
 int64_t
 do_blockdev_getsz (const char *device)
 {
-  return call_blockdev (device, "--getsz", 0, 1);
+  return call_blockdev (device, "--getsz", -1, 1);
 }
 
 int64_t
 do_blockdev_getsize64 (const char *device)
 {
-  return call_blockdev (device, "--getsize64", 0, 1);
+  return call_blockdev (device, "--getsize64", -1, 1);
 }
 
 int
 do_blockdev_flushbufs (const char *device)
 {
-  return call_blockdev (device, "--flushbufs", 0, 0);
+  return call_blockdev (device, "--flushbufs", -1, 0);
 }
 
 int
 do_blockdev_rereadpt (const char *device)
 {
-  return call_blockdev (device, "--rereadpt", 0, 0);
+  return call_blockdev (device, "--rereadpt", -1, 0);
 }
diff --git a/generator/actions.ml b/generator/actions.ml
index fe492e6..baa7679 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -12003,6 +12003,20 @@ This is the same as the C<lstat(2)> system
call." };
     longdesc = "\
 This is the internal call which implements C<guestfs_lstatnslist>."
};
 
+  { defaults with
+    name = "blockdev_setra";
+    style = RErr, [Device "device"; Int "sectors"], [];
+    proc_nr = Some 424;
+    tests = [
+      InitEmpty, Always, TestRun (
+        [["blockdev_setra"; "/dev/sda"; "1024"
]]), []
+    ];
+    shortdesc = "set readahead";
+    longdesc = "\
+Set readahead (in 512-byte sectors) for the device.
+
+This uses the L<blockdev(8)> command." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
-- 
2.1.0

Add support for a block mapping API, used by the 'virt-bmap' tool.
---
 daemon/Makefile.am   |   1 +
 daemon/bmap.c        | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++
 generator/actions.ml |  44 ++++++++++++++
 po/POTFILES          |   1 +
 src/MAX_PROC_NR      |   2 +-
 5 files changed, 215 insertions(+), 1 deletion(-)
 create mode 100644 daemon/bmap.c

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 8ccf322..b6444fa 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -86,6 +86,7 @@ guestfsd_SOURCES = \
 	blkdiscard.c \
 	blkid.c \
 	blockdev.c \
+	bmap.c \
 	btrfs.c \
 	cap.c \
 	checksum.c \
diff --git a/daemon/bmap.c b/daemon/bmap.c
new file mode 100644
index 0000000..555f776
--- /dev/null
+++ b/daemon/bmap.c
@@ -0,0 +1,168 @@
+/* libguestfs - the guestfsd daemon
+ * Copyright (C) 2014 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, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <dirent.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include "daemon.h"
+#include "actions.h"
+
+static int fd = -1;
+static DIR *dir = NULL;
+static struct stat statbuf;
+
+static void bmap_finalize (void) __attribute__((destructor));
+static void
+bmap_finalize (void)
+{
+  if (fd >= 0) {
+    close (fd);
+    fd = -1;
+  }
+  if (dir != NULL) {
+    closedir (dir);
+    dir = NULL;
+  }
+}
+
+static int
+bmap_prepare (const char *path, const char *orig_path)
+{
+  bmap_finalize ();
+
+  if (stat (path, &statbuf) == -1) {
+    reply_with_perror ("%s", orig_path);
+    return -1;
+  }
+
+  if (S_ISDIR (statbuf.st_mode)) {
+    /* Open a directory. */
+    dir = opendir (path);
+    if (dir == NULL) {
+      reply_with_perror ("opendir: %s", orig_path);
+      return -1;
+    }
+  }
+  else {
+    /* Open a regular file. */
+    fd = open (path, O_RDONLY | O_CLOEXEC);
+    if (fd == -1) {
+      reply_with_perror ("%s", orig_path);
+      return -1;
+    }
+
+    posix_fadvise (fd, 0, 0,
+                   POSIX_FADV_SEQUENTIAL |
+                   POSIX_FADV_NOREUSE |
+                   POSIX_FADV_DONTNEED);
+  }
+
+  return 0;
+}
+
+int
+do_bmap_file (const char *path)
+{
+  CLEANUP_FREE char *buf = NULL;
+
+  buf = sysroot_path (path);
+  if (!buf) {
+    reply_with_perror ("malloc");
+    return -1;
+  }
+
+  return bmap_prepare (buf, path);
+}
+
+int
+do_bmap_device (const char *device)
+{
+  return bmap_prepare (device, device);
+}
+
+static char buffer[BUFSIZ];
+
+int
+do_bmap (void)
+{
+  size_t n;
+  ssize_t r;
+  struct dirent *d;
+
+  /* Drop caches before starting the read. */
+  if (do_drop_caches (3) == -1)
+    return -1;
+
+  if (fd >= 0) {
+    n = statbuf.st_size;
+
+    while (n > 0) {
+      r = read (fd, buffer, n > BUFSIZ ? BUFSIZ : n);
+      if (r == -1) {
+        reply_with_perror ("read");
+        close (fd);
+        fd = -1;
+        return -1;
+      }
+      n -= r;
+    }
+
+    if (close (fd) == -1) {
+      reply_with_perror ("close");
+      fd = -1;
+      return -1;
+    }
+    fd = -1;
+  }
+
+  if (dir != NULL) {
+    for (;;) {
+      errno = 0;
+      d = readdir (dir);
+      if (!d) break;
+    }
+
+    /* Check readdir didn't fail */
+    if (errno != 0) {
+      reply_with_perror ("readdir");
+      closedir (dir);
+      dir = NULL;
+      return -1;
+    }
+
+    /* Close the directory handle */
+    if (closedir (dir) == -1) {
+      reply_with_perror ("closedir");
+      dir = NULL;
+      return -1;
+    }
+    dir = NULL;
+  }
+
+  return 0;
+}
diff --git a/generator/actions.ml b/generator/actions.ml
index baa7679..0f2e040 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -12017,6 +12017,50 @@ Set readahead (in 512-byte sectors) for the device.
 
 This uses the L<blockdev(8)> command." };
 
+  { defaults with
+    name = "bmap_file";
+    style = RErr, [Pathname "path"], [];
+    proc_nr = Some 425;
+    shortdesc = "prepare a file for block mapping";
+    longdesc = "\
+This is a specialized function used by L<virt-bmap(1)> to map which
+blocks are used by a file.  It is unlikely to be useful outside
+virt-bmap.
+
+This call prepares C<path> (which may be a regular file or directory)
+to be mapped.  You have to call C<guestfs_bmap> immediately
afterwards." };
+
+  { defaults with
+    name = "bmap_device";
+    style = RErr, [Device "device"], [];
+    proc_nr = Some 426;
+    shortdesc = "prepare a device for block mapping";
+    longdesc = "\
+This is a specialized function used by L<virt-bmap(1)> to map which
+blocks are used by a device.  It is unlikely to be useful outside
+virt-bmap.
+
+This call prepares C<device> to be mapped.  You have to call
C<guestfs_bmap>
+immediately afterwards." };
+
+  { defaults with
+    name = "bmap";
+    style = RErr, [], [];
+    proc_nr = Some 427;
+    shortdesc = "block map file or device";
+    longdesc = "\
+This is a specialized function used by L<virt-bmap(1)> to map which
+blocks are used by a file or device.  It is unlikely to be useful outside
+virt-bmap.
+
+This performs the specialized read of the file or device which was
+previously prepared (see C<guestfs_bmap_file> and
C<guestfs_bmap_device>).
+The file/directory/device is read from start to end without any caching
+or readahead.
+
+L<virt-bmap(1)> uses this call in conjunction with an nbdkit plugin
+which monitors which disk blocks are read during the operation." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
diff --git a/po/POTFILES b/po/POTFILES
index 1a088f5..b13cb86 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -22,6 +22,7 @@ daemon/base64.c
 daemon/blkdiscard.c
 daemon/blkid.c
 daemon/blockdev.c
+daemon/bmap.c
 daemon/btrfs.c
 daemon/cap.c
 daemon/checksum.c
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 5721413..d2a1e59 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-423
+427
-- 
2.1.0

On Sunday 23 November 2014 22:16:39 Richard W.M. Jones
wrote:
> Commit 588af1953e5f7ab74009b9175cc5d3efb8bb651a started with a very
> conservative list of permitted backing formats (just "raw" or
> "qcow2").  We can allow almost any format permitted by qemu, but
this
> commit just adds "vmdk" to this whitelist.
> ---
>  src/create.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/create.c b/src/create.c
> index 621ace5..bfb8b76 100644
> --- a/src/create.c
> +++ b/src/create.c
> @@ -261,7 +261,12 @@ disk_create_qcow2 (guestfs_h *g, const char
> *orig_filename, int64_t size,
> 
>    if (optargs->bitmask & GUESTFS_DISK_CREATE_BACKINGFORMAT_BITMASK)
{
>      backingformat = optargs->backingformat;
> -    if (STRNEQ (backingformat, "raw") && STRNEQ
(backingformat, "qcow2")) {
> +    /* Conservative whitelist.  This can be extended with other
> +     * valid formats as required.
> +     */
> +    if (STRNEQ (backingformat, "raw") &&
> +        STRNEQ (backingformat, "qcow2") &&
> +        STRNEQ (backingformat, "vmdk")) {
>        error (g, _("invalid value for backingformat parameter
'%s'"),
>               backingformat);
>        return -1;
LGTM.

-- 
Pino Toscano

On Sunday 23 November 2014 22:16:40 Richard W.M. Jones
wrote:
> This adds a binding for 'blockdev --setra', allowing you to adjust
the
> readahead parameter for filesystems and devices.
> ---
>  daemon/blockdev.c    | 30 ++++++++++++++++++++----------
>  generator/actions.ml | 14 ++++++++++++++
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/daemon/blockdev.c b/daemon/blockdev.c
> index 8a7b1a8..6e8821d 100644
> --- a/daemon/blockdev.c
> +++ b/daemon/blockdev.c
> @@ -62,7 +62,7 @@ call_blockdev (const char *device, const char
> *switc, int extraarg, int prints) */
>    udev_settle ();
> 
> -  if (extraarg > 0) {
> +  if (extraarg >= 0) {
>      snprintf (buf, sizeof buf, "%d", extraarg);
>      argv[2] = buf;
>      argv[3] = device;
> @@ -89,33 +89,43 @@ call_blockdev (const char *device, const char
> *switc, int extraarg, int prints) }
> 
>  int
> +do_blockdev_setra (const char *device, int sectors)
> +{
> +  if (sectors < 0) {
> +    reply_with_error ("readahead sectors must be >= 0");
> +    return -1;
> +  }
> +  return (int) call_blockdev (device, "--setra", sectors, 0);
> +}
> +
> +int
>  do_blockdev_setro (const char *device)
>  {
> -  return (int) call_blockdev (device, "--setro", 0, 0);
> +  return (int) call_blockdev (device, "--setro", -1, 0);
>  }
> 
>  int
>  do_blockdev_setrw (const char *device)
>  {
> -  return (int) call_blockdev (device, "--setrw", 0, 0);
> +  return (int) call_blockdev (device, "--setrw", -1, 0);
>  }
> 
>  int
>  do_blockdev_getro (const char *device)
>  {
> -  return (int) call_blockdev (device, "--getro", 0, 1);
> +  return (int) call_blockdev (device, "--getro", -1, 1);
>  }
> 
>  int
>  do_blockdev_getss (const char *device)
>  {
> -  return (int) call_blockdev (device, "--getss", 0, 1);
> +  return (int) call_blockdev (device, "--getss", -1, 1);
>  }
> 
>  int
>  do_blockdev_getbsz (const char *device)
>  {
> -  return (int) call_blockdev (device, "--getbsz", 0, 1);
> +  return (int) call_blockdev (device, "--getbsz", -1, 1);
>  }
> 
>  int
> @@ -128,23 +138,23 @@ do_blockdev_setbsz (const char *device, int
> blocksize) int64_t
>  do_blockdev_getsz (const char *device)
>  {
> -  return call_blockdev (device, "--getsz", 0, 1);
> +  return call_blockdev (device, "--getsz", -1, 1);
>  }
> 
>  int64_t
>  do_blockdev_getsize64 (const char *device)
>  {
> -  return call_blockdev (device, "--getsize64", 0, 1);
> +  return call_blockdev (device, "--getsize64", -1, 1);
>  }
> 
>  int
>  do_blockdev_flushbufs (const char *device)
>  {
> -  return call_blockdev (device, "--flushbufs", 0, 0);
> +  return call_blockdev (device, "--flushbufs", -1, 0);
>  }
> 
>  int
>  do_blockdev_rereadpt (const char *device)
>  {
> -  return call_blockdev (device, "--rereadpt", 0, 0);
> +  return call_blockdev (device, "--rereadpt", -1, 0);
>  }
> diff --git a/generator/actions.ml b/generator/actions.ml
> index fe492e6..baa7679 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -12003,6 +12003,20 @@ This is the same as the C<lstat(2)> system
> call." }; longdesc = "\
>  This is the internal call which implements
C<guestfs_lstatnslist>."
> };
> 
> +  { defaults with
> +    name = "blockdev_setra";
> +    style = RErr, [Device "device"; Int "sectors"],
[];
> +    proc_nr = Some 424;
> +    tests = [
> +      InitEmpty, Always, TestRun (
> +        [["blockdev_setra"; "/dev/sda";
"1024" ]]), []
> +    ];
> +    shortdesc = "set readahead";
> +    longdesc = "\
> +Set readahead (in 512-byte sectors) for the device.
> +
> +This uses the L<blockdev(8)> command." };
> +
>  ]
> 
>  (* Non-API meta-commands available only in guestfish.
Just a couple of notes:
- I would have done the small internal behaviour change of the
  extraarg parameter of call_blockdev in an own commit, so it isn't
  tangled with the API addition
- should there be some get counterpart, like blockdev_getra?
- don't forget to commit MAX_PROC_NR together with this

Otherwise, LGTM.

-- 
Pino Toscano

On Sunday 23 November 2014 22:16:41 Richard W.M. Jones
wrote:
> Add support for a block mapping API, used by the 'virt-bmap' tool.
> ---
>  daemon/Makefile.am   |   1 +
>  daemon/bmap.c        | 168
+++++++++++++++++++++++++++++++++++++++++++++++++++
>  generator/actions.ml |  44 ++++++++++++++
>  po/POTFILES          |   1 +
>  src/MAX_PROC_NR      |   2 +-
>  5 files changed, 215 insertions(+), 1 deletion(-)
>  create mode 100644 daemon/bmap.c
> 
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 8ccf322..b6444fa 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -86,6 +86,7 @@ guestfsd_SOURCES = \
>         blkdiscard.c \
>         blkid.c \
>         blockdev.c \
> +       bmap.c \
>         btrfs.c \
>         cap.c \
>         checksum.c \
> diff --git a/daemon/bmap.c b/daemon/bmap.c
> new file mode 100644
> index 0000000..555f776
> --- /dev/null
> +++ b/daemon/bmap.c
> @@ -0,0 +1,168 @@
> +/* libguestfs - the guestfsd daemon
> + * Copyright (C) 2014 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, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <dirent.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include "daemon.h"
> +#include "actions.h"
> +
> +static int fd = -1;
> +static DIR *dir = NULL;
> +static struct stat statbuf;
> +
> +static void bmap_finalize (void) __attribute__((destructor));
> +static void
> +bmap_finalize (void)
> +{
> +  if (fd >= 0) {
> +    close (fd);
> +    fd = -1;
> +  }
> +  if (dir != NULL) {
> +    closedir (dir);
> +    dir = NULL;
> +  }
> +}
> +
> +static int
> +bmap_prepare (const char *path, const char *orig_path)
> +{
> +  bmap_finalize ();
> +
> +  if (stat (path, &statbuf) == -1) {
> +    reply_with_perror ("%s", orig_path);
> +    return -1;
> +  }
> +
> +  if (S_ISDIR (statbuf.st_mode)) {
> +    /* Open a directory. */
> +    dir = opendir (path);
> +    if (dir == NULL) {
> +      reply_with_perror ("opendir: %s", orig_path);
> +      return -1;
> +    }
> +  }
> +  else {
> +    /* Open a regular file. */
> +    fd = open (path, O_RDONLY | O_CLOEXEC);
> +    if (fd == -1) {
> +      reply_with_perror ("%s", orig_path);
> +      return -1;
> +    }
Wouldn't be better to just always open the file, stat the fd, and
if it's a directory fdopendir + close it?
> +
> +    posix_fadvise (fd, 0, 0,
> +                   POSIX_FADV_SEQUENTIAL |
> +                   POSIX_FADV_NOREUSE |
> +                   POSIX_FADV_DONTNEED);
> +  }
> +
> +  return 0;
> +}
> +
> +int
> +do_bmap_file (const char *path)
> +{
> +  CLEANUP_FREE char *buf = NULL;
> +
> +  buf = sysroot_path (path);
> +  if (!buf) {
> +    reply_with_perror ("malloc");
> +    return -1;
> +  }
> +
> +  return bmap_prepare (buf, path);
> +}
> +
> +int
> +do_bmap_device (const char *device)
> +{
> +  return bmap_prepare (device, device);
> +}
> +
> +static char buffer[BUFSIZ];
> +
> +int
> +do_bmap (void)
> +{
> +  size_t n;
> +  ssize_t r;
> +  struct dirent *d;
> +
> +  /* Drop caches before starting the read. */
> +  if (do_drop_caches (3) == -1)
> +    return -1;
> +
> +  if (fd >= 0) {
> +    n = statbuf.st_size;
> +
> +    while (n > 0) {
> +      r = read (fd, buffer, n > BUFSIZ ? BUFSIZ : n);
> +      if (r == -1) {
> +        reply_with_perror ("read");
> +        close (fd);
> +        fd = -1;
> +        return -1;
> +      }
> +      n -= r;
> +    }
> +
> +    if (close (fd) == -1) {
> +      reply_with_perror ("close");
> +      fd = -1;
> +      return -1;
> +    }
> +    fd = -1;
> +  }
> +
> +  if (dir != NULL) {
> +    for (;;) {
> +      errno = 0;
> +      d = readdir (dir);
> +      if (!d) break;
> +    }
> +
> +    /* Check readdir didn't fail */
> +    if (errno != 0) {
> +      reply_with_perror ("readdir");
> +      closedir (dir);
> +      dir = NULL;
> +      return -1;
> +    }
> +
> +    /* Close the directory handle */
> +    if (closedir (dir) == -1) {
> +      reply_with_perror ("closedir");
> +      dir = NULL;
> +      return -1;
> +    }
> +    dir = NULL;
> +  }
> +
> +  return 0;
> +}
> diff --git a/generator/actions.ml b/generator/actions.ml
> index baa7679..0f2e040 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -12017,6 +12017,50 @@ Set readahead (in 512-byte sectors) for the
device.
>  
>  This uses the L<blockdev(8)> command." };
>  
> +  { defaults with
> +    name = "bmap_file";
> +    style = RErr, [Pathname "path"], [];
> +    proc_nr = Some 425;
> +    shortdesc = "prepare a file for block mapping";
> +    longdesc = "\
> +This is a specialized function used by L<virt-bmap(1)> to map which
> +blocks are used by a file.  It is unlikely to be useful outside
> +virt-bmap.
> +
> +This call prepares C<path> (which may be a regular file or
directory)
> +to be mapped.  You have to call C<guestfs_bmap> immediately
afterwards." };
> +
> +  { defaults with
> +    name = "bmap_device";
> +    style = RErr, [Device "device"], [];
> +    proc_nr = Some 426;
> +    shortdesc = "prepare a device for block mapping";
> +    longdesc = "\
> +This is a specialized function used by L<virt-bmap(1)> to map which
> +blocks are used by a device.  It is unlikely to be useful outside
> +virt-bmap.
> +
> +This call prepares C<device> to be mapped.  You have to call
C<guestfs_bmap>
> +immediately afterwards." };
> +
> +  { defaults with
> +    name = "bmap";
> +    style = RErr, [], [];
> +    proc_nr = Some 427;
> +    shortdesc = "block map file or device";
> +    longdesc = "\
> +This is a specialized function used by L<virt-bmap(1)> to map which
> +blocks are used by a file or device.  It is unlikely to be useful outside
> +virt-bmap.
> +
> +This performs the specialized read of the file or device which was
> +previously prepared (see C<guestfs_bmap_file> and
C<guestfs_bmap_device>).
> +The file/directory/device is read from start to end without any caching
> +or readahead.
> +
> +L<virt-bmap(1)> uses this call in conjunction with an nbdkit plugin
> +which monitors which disk blocks are read during the operation." };
> +
>  ]
I'm missing what virt-bmap would do internally, but why the need for
separate prepare + execute actions, instead of bmap_file/device doing
the actual work? This could also avoid having them stateful.

-- 
Pino Toscano

After a chat with some people at Red Hat, I've decided to withdraw the
'bmap*' APIs.

I think we may be able to do this better by having a go at mapping
blocks through the layers.  We only really need to cover FIEMAP
(filesystem extent mapping API), device-mapper, and partitions.

This ignores MD (RAID etc) but that's not very common on VMs.  It also
means we're mapping through to the virtual disk address, so ignoring
the container format underneath (qcow2 etc).

Unfortunately ntfs-3g does not have the FIEMAP ioctl, so we'd need to
add that.

In other words - a chunk of work, but it could mean that we can get
accurate block maps.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html