[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH v2] Allow 9p filesystems to be listed and mounted (RHBZ#714981).



On Wed, Jun 22, 2011 at 02:06:27PM +0100, Richard W.M. Jones wrote:
> Second version.  I have tested these and they work.
> 
> The earlier version of the patch which modified the mount-vfs call so
> that the Device parameter became a String would not have worked
> properly.  It would mean that device name translation[1] would not
> have been done on that parameter any longer.
> 
> I also considered changing all the mount calls so that they take a
> string instead of a device name, and then doing device name
> translation manually (ie. calling RESOLVE_DEVICE conditionally, as is
> done in do_umount[2]).  This would also allow bind mounts, but would
> also lead to a very complex do_mount_vfs[3] implementation.
> 
> So this implements a separate API: guestfs_mount_9p, specifically for
> this purpose of mounting 9p filesystems discovered through
> guestfs_list_9p.  The advantage of having an API is it's simple and
> specialized, and we can deprecate it later if we find a better way to
> do it.
> 
> Please test it, and tell me if it works for you.
> 
> Rich.
> 
> [1] http://libguestfs.org/guestfs.3.html#algorithm_for_block_device_name_translation
> [2] http://git.annexia.org/?p=libguestfs.git;a=blob;f=daemon/mount.c;h=be289dade5865327f65bea16481c733c318b1173;hb=HEAD#l145
> [3] http://git.annexia.org/?p=libguestfs.git;a=blob;f=daemon/mount.c;h=be289dade5865327f65bea16481c733c318b1173;hb=HEAD#l77
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> virt-p2v converts physical machines to virtual machines.  Boot with a
> live CD or over the network (PXE) and turn machines into Xen guests.
> http://et.redhat.com/~rjones/virt-p2v

> From 5f10c3350338bbca735a74db26f98da968957bd9 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones redhat com>
> Date: Wed, 22 Jun 2011 10:13:23 +0100
> Subject: [PATCH 1/2] New API: list-9p lists 9p filesystem mount tags
>  (RHBZ#714981).
> 
> ---
>  daemon/9p.c                    |  170 ++++++++++++++++++++++++++++++++++++++++
>  daemon/Makefile.am             |    1 +
>  generator/generator_actions.ml |    7 ++
>  po/POTFILES.in                 |    1 +
>  src/MAX_PROC_NR                |    2 +-
>  5 files changed, 180 insertions(+), 1 deletions(-)
>  create mode 100644 daemon/9p.c
> 
> diff --git a/daemon/9p.c b/daemon/9p.c
> new file mode 100644
> index 0000000..bc95803
> --- /dev/null
> +++ b/daemon/9p.c
> @@ -0,0 +1,170 @@
> +/* libguestfs - the guestfsd daemon
> + * Copyright (C) 2011 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <dirent.h>
> +#include <fcntl.h>
> +
> +#include "daemon.h"
> +#include "actions.h"
> +
> +#define BUS_PATH "/sys/bus/virtio/drivers/9pnet_virtio"
> +
> +static char *read_whole_file (const char *filename);
> +
> +/* https://bugzilla.redhat.com/show_bug.cgi?id=714981#c1 */
> +char **
> +do_list_9p (void)
> +{
> +  char **r = NULL;
> +  int size = 0, alloc = 0;
> +
> +  DIR *dir;
> +  int err = 0;
> +
> +  dir = opendir (BUS_PATH);
> +  if (!dir) {
> +    perror ("opendir: " BUS_PATH);
> +    if (errno != ENOENT)
> +      return NULL;
> +
> +    /* If this directory doesn't exist, it probably means that
> +     * the virtio driver isn't loaded.  Don't return an error
> +     * in this case, but return an empty list.
> +     */
> +    if (add_string (&r, &size, &alloc, NULL) == -1)
> +      return NULL;
> +
> +    return r;
> +  }
> +
> +  while (1) {
> +    errno = 0;
> +    struct dirent *d = readdir (dir);
> +    if (d == NULL) break;
> +
> +    if (STRPREFIX (d->d_name, "virtio")) {
> +      char mount_tag_path[256];
> +      snprintf (mount_tag_path, sizeof mount_tag_path,
> +                BUS_PATH "/%s/mount_tag", d->d_name);
> +
> +      /* A bit unclear, but it looks like the virtio transport allows
> +       * the mount tag length to be unlimited (or up to 65536 bytes).
> +       * See: linux/include/linux/virtio_9p.h
> +       */
> +      char *mount_tag = read_whole_file (mount_tag_path);
> +      if (mount_tag == 0)
> +        continue;
> +
> +      if (add_string (&r, &size, &alloc, mount_tag) == -1) {
> +        free (mount_tag);
> +        free_stringslen (r, size);
> +        closedir (dir);
> +        return NULL;
> +      }
> +
> +      free (mount_tag);
> +    }
> +  }
> +
> +  /* Check readdir didn't fail */
> +  if (errno != 0) {
> +    reply_with_perror ("readdir: /sys/block");
> +    free_stringslen (r, size);
> +    closedir (dir);
> +    return NULL;
> +  }
> +
> +  /* Close the directory handle */
> +  if (closedir (dir) == -1) {
> +    reply_with_perror ("closedir: /sys/block");
> +    free_stringslen (r, size);
> +    return NULL;
> +  }
> +
> +  /* Sort the tags.  Note that r might be NULL if there are no tags. */
> +  if (r != NULL)
> +    sort_strings (r, size);
> +
> +  /* NULL terminate the list */
> +  if (add_string (&r, &size, &alloc, NULL) == -1)
> +    return NULL;
> +
> +  return r;
> +}
> +
> +/* Read whole file into dynamically allocated array.  If there is an
> + * error, DON'T call reply_with_perror, just return NULL.  Returns a
> + * \0-terminated string.
> + */
> +static char *
> +read_whole_file (const char *filename)
> +{
> +  char *r = NULL;
> +  size_t alloc = 0, size = 0;
> +  int fd;
> +
> +  fd = open (filename, O_RDONLY);
> +  if (fd == -1) {
> +    perror (filename);
> +    return NULL;
> +  }
> +
> +  while (1) {
> +    alloc += 256;
> +    char *r2 = realloc (r, alloc);
> +    if (r2 == NULL) {
> +      perror ("realloc");
> +      free (r);
> +      return NULL;
> +    }
> +    r = r2;
> +
> +    /* The '- 1' in the size calculation ensures there is space below
> +     * to add \0 to the end of the input.
> +     */
> +    ssize_t n = read (fd, r + size, alloc - size - 1);
> +    if (n == -1) {
> +      perror (filename);
> +      free (r);
> +      return NULL;
> +    }
> +    if (n == 0)
> +      break;
> +    size += n;
> +  }
> +
> +  if (close (fd) == -1) {
> +    perror (filename);
> +    free (r);
> +    return NULL;
> +  }
> +
> +  r[size] = '\0';
> +
> +  return r;
> +}
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 8fb070f..78049c9 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -88,6 +88,7 @@ errnostring.h: $(libsrcdir)/errnostring.h
>  
>  noinst_PROGRAMS = guestfsd
>  guestfsd_SOURCES = \
> +	9p.c \
>  	actions.h \
>  	available.c \
>  	augeas.c \
> diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
> index 74b6515..634ceeb 100644
> --- a/generator/generator_actions.ml
> +++ b/generator/generator_actions.ml
> @@ -5960,6 +5960,13 @@ This returns true iff the device exists and contains all zero bytes.
>  
>  Note that for large devices this can take a long time to run.");
>  
> +  ("list_9p", (RStringList "mounttags", [], []), 285, [],
> +   [],
> +   "list 9p filesystems",
> +   "\
> +List all 9p filesystems attached to the guest.  A list of
> +mount tags is returned.");
> +
>  ]
>  
>  let all_functions = non_daemon_functions @ daemon_functions
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 7c0df52..da47c91 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -1,6 +1,7 @@
>  cat/virt-cat.c
>  cat/virt-filesystems.c
>  cat/virt-ls.c
> +daemon/9p.c
>  daemon/augeas.c
>  daemon/available.c
>  daemon/base64.c
> diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> index c9716b7..6cf4452 100644
> --- a/src/MAX_PROC_NR
> +++ b/src/MAX_PROC_NR
> @@ -1 +1 @@
> -284
> +285
> -- 
> 1.7.5.2
> 

> From dd422b9797b2b73764bb04ec46aebbf7b08cb9d8 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones redhat com>
> Date: Wed, 22 Jun 2011 13:55:14 +0100
> Subject: [PATCH 2/2] New API: mount-9p lets you mount 9p filesystems
>  (RHBZ#714981).
> 
> ---
>  daemon/9p.c                    |   55 ++++++++++++++++++++++++++++++++++++++++
>  generator/generator_actions.ml |   11 ++++++++
>  src/MAX_PROC_NR                |    2 +-
>  3 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/daemon/9p.c b/daemon/9p.c
> index bc95803..27b2230 100644
> --- a/daemon/9p.c
> +++ b/daemon/9p.c
> @@ -168,3 +168,58 @@ read_whole_file (const char *filename)
>  
>    return r;
>  }
> +
> +int
> +do_mount_9p (const char *options, const char *mount_tag, const char *mountpoint)
> +{
> +  char *mp = NULL, *opts = NULL, *err = NULL;
> +  struct stat statbuf;
> +  int r = -1;
> +
> +  ABS_PATH (mountpoint, , return -1);
> +
> +  mp = sysroot_path (mountpoint);
> +  if (!mp) {
> +    reply_with_perror ("malloc");
> +    goto out;
> +  }
> +
> +  /* Check the mountpoint exists and is a directory. */
> +  if (stat (mp, &statbuf) == -1) {
> +    reply_with_perror ("%s", mountpoint);
> +    goto out;
> +  }
> +  if (!S_ISDIR (statbuf.st_mode)) {
> +    reply_with_perror ("%s: mount point is not a directory", mountpoint);
> +    goto out;
> +  }
> +
> +  /* Add trans=virtio to the options. */
> +  if (STREQ (options, "")) {
> +    opts = strdup ("trans=virtio");
> +    if (opts == NULL) {
> +      reply_with_perror ("strdup");
> +      goto out;
> +    }
> +  }
> +  else {
> +    if (asprintf (&opts, "trans=virtio,%s", options) == -1) {
> +      reply_with_perror ("asprintf");
> +      goto out;
> +    }
> +  }
> +
> +  r = command (NULL, &err,
> +               "mount", "-o", opts, "-t", "9p", mount_tag, mp, NULL);
> +  if (r == -1) {
> +    reply_with_error ("%s on %s: %s", mount_tag, mountpoint, err);
> +    goto out;
> +  }
> +
> +  r = 0;
> + out:
> +  free (err);
> +  free (opts);
> +  free (mp);
> +  return r;
> +}
> diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
> index 634ceeb..27a02d0 100644
> --- a/generator/generator_actions.ml
> +++ b/generator/generator_actions.ml
> @@ -5967,6 +5967,17 @@ Note that for large devices this can take a long time to run.");
>  List all 9p filesystems attached to the guest.  A list of
>  mount tags is returned.");
>  
> +  ("mount_9p", (RErr, [String "options"; String "mounttag"; String "mountpoint"], []), 286, [],
> +   [],
> +   "mount 9p filesystem",
> +   "\
> +Mount the virtio-9p filesystem with the tag C<mounttag> on the
> +directory C<mountpoint>.
> +
> +If required, C<trans=virtio> will be automatically added to the options.
> +Any other options required can be passed in the C<options> parameter,
> +or this can be left as an empty string.");

Is it possible to make 'options' the 3rd parameter, and optional,
so you don't need todo

   ><fs> mount-9p "" org.kernel.other /tmp/

Which is the common case.

Aside from that, the patch works fine

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]