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

Re: [Libguestfs] [PATCH 3/3] mountable: Implement Mountable support for all apis which take it



On Thu, Jan 24, 2013 at 10:19:50AM +0000, Matthew Booth wrote:
> A Mountable is passed from the library to the daemon as a string. The daemon
> stub parses it into a mountable_t, which it passes to the implementation.
> 
> Update all implementations which now take a mountable_t.
> ---
>  daemon/blkid.c      | 36 ++++++++++++++++++++++++++++++------
>  daemon/daemon.h     | 39 +++++++++++++++++++++++++++++++++++++++
>  daemon/ext2.c       | 15 ++++++++++++---
>  daemon/guestfsd.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  daemon/labels.c     | 19 +++++++++++++++++--
>  daemon/mount.c      | 52 +++++++++++++++++++++++++++++++++++++++++++---------
>  generator/c.ml      | 12 +++++++++++-
>  generator/daemon.ml | 11 ++++++++---
>  8 files changed, 202 insertions(+), 24 deletions(-)
> 
> diff --git a/daemon/blkid.c b/daemon/blkid.c
> index b6bc22d..429a005 100644
> --- a/daemon/blkid.c
> +++ b/daemon/blkid.c
> @@ -69,21 +69,45 @@ get_blkid_tag (const char *device, const char *tag)
>  }
>  
>  char *
> -do_vfs_type (const char *device)
> +do_vfs_type (const mountable_t *mountable)
>  {
> -  return get_blkid_tag (device, "TYPE");
> +  switch (mountable->type) {
> +    case MOUNTABLE_DEVICE:
> +      return get_blkid_tag (mountable->desc.device, "TYPE");
> +    case MOUNTABLE_BTRFSVOL:
> +      return strdup("btrfs");

Please put a space between the function name and the opening
parenthesis.

You need to check the return value from strdup, always.

However I think this code is wrong.  It should still call
get_blkid_tag on the device part of the mountable string, to avoid
surprises.

> +    default:
> +      reply_with_error ("Unknown mountable type: %i", mountable->type);
> +      return NULL;
> +  }
>  }
>  
>  char *
> -do_vfs_label (const char *device)
> +do_vfs_label (const mountable_t *mountable)
>  {
> -  return get_blkid_tag (device, "LABEL");
> +  switch (mountable->type) {
> +    case MOUNTABLE_DEVICE:
> +      return get_blkid_tag (mountable->desc.device, "LABEL");
> +    case MOUNTABLE_BTRFSVOL:
> +      return get_blkid_tag (mountable->desc.btrfsvol.device, "LABEL");

If we had a common mountable->device field, a lot of this code
would be much simpler.

> +    default:
> +      reply_with_error ("Unknown mountable type: %i", mountable->type);
> +      return NULL;
> +  }
>  }
>  
>  char *
> -do_vfs_uuid (const char *device)
> +do_vfs_uuid (const mountable_t *mountable)
>  {
> -  return get_blkid_tag (device, "UUID");
> +  switch (mountable->type) {
> +    case MOUNTABLE_DEVICE:
> +      return get_blkid_tag (mountable->desc.device, "UUID");
> +    case MOUNTABLE_BTRFSVOL:
> +      return get_blkid_tag (mountable->desc.btrfsvol.device, "UUID");
> +    default:
> +      reply_with_error ("Unknown mountable type: %i", mountable->type);
> +      return NULL;
> +  }
>  }
>  
>  /* RHEL5 blkid doesn't have the -p (low-level probing) option and the
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index df1ba3a..8f5015a 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -47,6 +47,24 @@ extern int xwrite (int sock, const void *buf, size_t len)
>  extern int xread (int sock, void *buf, size_t len)
>    __attribute__((__warn_unused_result__));
>  
> +/* Mountables */
> +
> +typedef enum {
> +  MOUNTABLE_DEVICE,
> +  MOUNTABLE_BTRFSVOL
> +} mountable_type_t;
> +
> +typedef struct {
> +  mountable_type_t type;
> +  union {
> +    const char *device;
> +    struct {
> +      const char *device;
> +      const char *volume;
> +    } btrfsvol;
> +  } desc;
> +} mountable_t;
> +
>  /* Growable strings buffer. */
>  struct stringsbuf {
>    char **argv;
> @@ -114,6 +132,8 @@ extern void trim (char *str);
>  
>  extern int device_name_translation (char *device);
>  
> +extern int parse_btrfsvol (char *desc, mountable_t *mountable);
> +
>  extern int prog_exists (const char *prog);
>  
>  extern void udev_settle (void);
> @@ -320,6 +340,25 @@ is_zero (const char *buffer, size_t size)
>      }                                                                   \
>    } while (0)
>  
> +#define RESOLVE_MOUNTABLE(string,mountable,cancel_stmt,fail_stmt)       \
> +  do {                                                                  \
> +    if (strncmp ((string), "btrfsvol:", strlen ("btrfsvol:")) == 0) {   \
> +      if (parse_btrfsvol ((string) + strlen ("btrfsvol:"), &(mountable)) == -1)\
> +      {                                                                 \
> +        cancel_stmt;                                                    \
> +        reply_with_error ("%s: %s: expecting a btrfs volume",           \
> +                          __func__, (string));                          \
> +        fail_stmt;                                                      \
> +      }                                                                 \
> +    }                                                                   \
> +                                                                        \
> +    else {                                                              \
> +      mountable.type = MOUNTABLE_DEVICE;                                \
> +      mountable.desc.device = (string);                                 \
> +      RESOLVE_DEVICE((string), cancel_stmt, fail_stmt);                 \
> +    }                                                                   \
> +  } while (0)
> +
>  /* Helper for functions which need either an absolute path in the
>   * mounted filesystem, OR a /dev/ device which exists.
>   *
> diff --git a/daemon/ext2.c b/daemon/ext2.c
> index ab879db..94e85f0 100644
> --- a/daemon/ext2.c
> +++ b/daemon/ext2.c
> @@ -127,13 +127,19 @@ do_tune2fs_l (const char *device)
>  int
>  do_set_e2label (const char *device, const char *label)
>  {
> -  return do_set_label (device, label);
> +  mountable_t mountable;
> +  mountable.type = MOUNTABLE_DEVICE;
> +  mountable.desc.device = device;
> +  return do_set_label (&mountable, label);
>  }
>  
>  char *
>  do_get_e2label (const char *device)
>  {
> -  return do_vfs_label (device);
> +  mountable_t mountable;
> +  mountable.type = MOUNTABLE_DEVICE;
> +  mountable.desc.device = device;
> +  return do_vfs_label (&mountable);
>  }
>  
>  int
> @@ -156,7 +162,10 @@ do_set_e2uuid (const char *device, const char *uuid)
>  char *
>  do_get_e2uuid (const char *device)
>  {
> -  return do_vfs_uuid (device);
> +  mountable_t mountable;
> +  mountable.type = MOUNTABLE_DEVICE;
> +  mountable.desc.device = device;
> +  return do_vfs_uuid (&mountable);
>  }
>  
>  /* If the filesystem is not mounted, run e2fsck -f on it unconditionally. */
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index 9c1c028..5016780 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -1165,6 +1165,48 @@ device_name_translation (char *device)
>    return -1;
>  }
>  
> +/* Parse the mountable descriptor for a btrfs subvolume.  Don't call this
> + * directly - use the RESOLVE_MOUNTABLE macro.
> + *
> + * A btrfs subvolume is given as:
> + *
> + * btrfsvol:/dev/sda3/root
> + *
> + * where /dev/sda3 is a block device containing a btrfs filesystem, and root is
> + * the name of a subvolume on it. This function is passed the string following
> + * 'btrfsvol:'.
> + */
> +int
> +parse_btrfsvol (char *desc, mountable_t *mountable)
> +{
> +  mountable->type = MOUNTABLE_BTRFSVOL;
> +
> +  char *device = desc;
> +
> +  if (strncmp (device, "/dev/", strlen("/dev/")) == -1)
> +    return -1;
> +
> +  char *volume = NULL;
> +  char *slash = device + strlen("/dev/") - 1;
> +  while ((slash = strchr (slash + 1, '/'))) {
> +    *slash = '\0';
> +
> +    if (!is_root_device(device) && device_name_translation (device) == 0) {
> +      volume = slash + 1;
> +      break;
> +    }
> +
> +    *slash = '/';
> +  }
> +
> +  if (!volume) return -1;
> +
> +  mountable->desc.btrfsvol.device = device;
> +  mountable->desc.btrfsvol.volume = volume;
> +
> +  return 0;
> +}
> +
>  /* Check program exists and is executable on $PATH.  Actually, we
>   * just assume PATH contains the default entries (see main() above).
>   */
> diff --git a/daemon/labels.c b/daemon/labels.c
> index ead6b46..e2f045a 100644
> --- a/daemon/labels.c
> +++ b/daemon/labels.c
> @@ -75,16 +75,31 @@ ntfslabel (const char *device, const char *label)
>  }
>  
>  int
> -do_set_label (const char *device, const char *label)
> +do_set_label (const mountable_t *mountable, const char *label)
>  {
>    char *vfs_type;
>    int r;
>  
>    /* How we set the label depends on the filesystem type. */
> -  vfs_type = do_vfs_type (device);
> +  vfs_type = do_vfs_type (mountable);
>    if (vfs_type == NULL)
>      return -1;
>  
> +  const char *device;
> +  switch (mountable->type) {
> +    case MOUNTABLE_DEVICE:
> +      device = mountable->desc.device;
> +      break;
> +    
> +    case MOUNTABLE_BTRFSVOL:
> +      device = mountable->desc.btrfsvol.device;
> +      break;
> +
> +    default:
> +      reply_with_error ("Invalid mountable type %i", mountable->type);
> +      return -1;
> +  }
> +
>    if (STREQ (vfs_type, "ext2") || STREQ (vfs_type, "ext3")
>        || STREQ (vfs_type, "ext4"))
>      r = e2label (device, label);
> diff --git a/daemon/mount.c b/daemon/mount.c
> index c84faaf..d0c321a 100644
> --- a/daemon/mount.c
> +++ b/daemon/mount.c
> @@ -124,7 +124,7 @@ is_device_mounted (const char *device)
>  
>  int
>  do_mount_vfs (const char *options, const char *vfstype,
> -              const char *device, const char *mountpoint)
> +              const mountable_t *mountable, const char *mountpoint)
>  {
>    int r;
>    char *mp;
> @@ -151,13 +151,47 @@ do_mount_vfs (const char *options, const char *vfstype,
>      return -1;
>    }
>  
> +  char *options_plus = NULL;
> +  const char *device = NULL;
> +  switch (mountable->type) {
> +    case MOUNTABLE_DEVICE:
> +      device = mountable->desc.device;
> +      break;
> +
> +    case MOUNTABLE_BTRFSVOL:
> +      device = mountable->desc.btrfsvol.device;
> +      if (options && strlen(options) > 1) {
> +        if (asprintf (&options_plus, "subvol=%s,%s",
> +                      mountable->desc.btrfsvol.volume, options) == -1)
> +        {
> +          reply_with_perror ("asprintf");
> +          return -1;
> +        }
> +      } else {
> +        if (asprintf (&options_plus, "subvol=%s",
> +                      mountable->desc.btrfsvol.volume) == -1)
> +        {
> +          reply_with_perror ("asprintf");
> +          return -1;
> +        }
> +      }
> +      break;
> +
> +    default:
> +      reply_with_error ("Invalid mountable type %i", mountable->type);
> +      return -1;
> +  }
> +
>    if (vfstype)
>      r = command (NULL, &error,
> -                 str_mount, "-o", options, "-t", vfstype, device, mp, NULL);
> +                 str_mount, "-o", options_plus ? options_plus : options,
> +                 "-t", vfstype, device, mp, NULL);
>    else
>      r = command (NULL, &error,
> -                 str_mount, "-o", options, device, mp, NULL);
> +                 str_mount, "-o", options_plus ? options_plus : options,
> +                 device, mp, NULL);
>    free (mp);
> +  free (options_plus);
>    if (r == -1) {
>      reply_with_error ("%s on %s (options: '%s'): %s",
>                        device, mountpoint, options, error);
> @@ -170,22 +204,22 @@ do_mount_vfs (const char *options, const char *vfstype,
>  }
>  
>  int
> -do_mount (const char *device, const char *mountpoint)
> +do_mount (const mountable_t *mountable, const char *mountpoint)
>  {
> -  return do_mount_vfs ("", NULL, device, mountpoint);
> +  return do_mount_vfs ("", NULL, mountable, mountpoint);
>  }
>  
>  int
> -do_mount_ro (const char *device, const char *mountpoint)
> +do_mount_ro (const mountable_t *mountable, const char *mountpoint)
>  {
> -  return do_mount_vfs ("ro", NULL, device, mountpoint);
> +  return do_mount_vfs ("ro", NULL, mountable, mountpoint);
>  }
>  
>  int
> -do_mount_options (const char *options, const char *device,
> +do_mount_options (const char *options, const mountable_t *mountable,
>                    const char *mountpoint)
>  {
> -  return do_mount_vfs (options, NULL, device, mountpoint);
> +  return do_mount_vfs (options, NULL, mountable, mountpoint);
>  }
>  
>  /* Takes optional arguments, consult optargs_bitmask. */
> diff --git a/generator/c.ml b/generator/c.ml
> index c20f502..e0fc852 100644
> --- a/generator/c.ml
> +++ b/generator/c.ml
> @@ -109,12 +109,18 @@ let rec generate_prototype ?(extern = true) ?(static = false)
>      List.iter (
>        function
>        | Pathname n
> -      | Device n | Mountable n | Dev_or_Path n
> +      | Device n | Dev_or_Path n
>        | String n
>        | OptString n
>        | Key n ->
>            next ();
>            pr "const char *%s" n
> +      | Mountable n ->
> +          next();
> +          if in_daemon then
> +            pr "const mountable_t *%s" n
> +          else
> +            pr "const char *%s" n
>        | StringList n | DeviceList n ->
>            next ();
>            pr "char *const *%s" n
> @@ -148,6 +154,7 @@ let rec generate_prototype ?(extern = true) ?(static = false)
>  
>  (* Generate C call arguments, eg "(handle, foo, bar)" *)
>  and generate_c_call_args ?handle ?(implicit_size_ptr = "&size")
> +    ?(in_daemon = false)
>      (ret, args, optargs) =
>    pr "(";
>    let comma = ref false in
> @@ -164,6 +171,9 @@ and generate_c_call_args ?handle ?(implicit_size_ptr = "&size")
>      | BufferIn n ->
>          next ();
>          pr "%s, %s_size" n n
> +    | Mountable n ->
> +        next ();
> +        pr (if in_daemon then "&%s" else "%s") n
>      | arg ->
>          next ();
>          pr "%s" (name_of_argt arg)
> diff --git a/generator/daemon.ml b/generator/daemon.ml
> index a7096a1..094fe6d 100644
> --- a/generator/daemon.ml
> +++ b/generator/daemon.ml
> @@ -38,6 +38,7 @@ let generate_daemon_actions_h () =
>    pr "\n";
>  
>    pr "#include \"guestfs_protocol.h\"\n";
> +  pr "#include \"daemon.h\"\n";
>    pr "\n";
>  
>    List.iter (
> @@ -111,11 +112,12 @@ and generate_daemon_actions () =
>          pr "  struct guestfs_%s_args args;\n" name;
>          List.iter (
>            function
> -          | Device n | Mountable n | Dev_or_Path n
> +          | Device n | Dev_or_Path n
>            | Pathname n
>            | String n
>            | Key n
>            | OptString n -> pr "  char *%s;\n" n
> +          | Mountable n -> pr "  mountable_t %s;\n" n
>            | StringList n | DeviceList n -> pr "  char **%s;\n" n
>            | Bool n -> pr "  int %s;\n" n
>            | Int n -> pr "  int %s;\n" n
> @@ -205,10 +207,13 @@ and generate_daemon_actions () =
>                pr_args n;
>                pr "  ABS_PATH (%s, %s, goto done);\n"
>                  n (if is_filein then "cancel_receive ()" else "");
> -          | Device n | Mountable n ->
> +          | Device n ->
>                pr_args n;
>                pr "  RESOLVE_DEVICE (%s, %s, goto done);\n"
>                  n (if is_filein then "cancel_receive ()" else "");
> +          | Mountable n ->
> +              pr "  RESOLVE_MOUNTABLE(args.%s, %s, %s, goto done);\n"
> +                n n (if is_filein then "cancel_receive ()" else "");
>            | Dev_or_Path n ->
>                pr_args n;
>                pr "  REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, %s, goto done);\n"
> @@ -257,7 +262,7 @@ and generate_daemon_actions () =
>              (function FileIn _ | FileOut _ -> false | _ -> true) args in
>          let style = ret, args' @ args_of_optargs optargs, [] in
>          pr "  r = do_%s " name;
> -        generate_c_call_args style;
> +        generate_c_call_args ~in_daemon:true style;
>          pr ";\n" in
>  
>        (match ret with

Seems generally good, but I think a common device field would make the
code changes smaller.  It's an internal detail which we could change
later.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW


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