[Libguestfs] [PATCH 3/3] New APIs: bmap-file, bmap-device, bmap.

[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