[Libguestfs] [PATCH 3/3] mountable: Implement Mountable support for all apis which take it
Richard W.M. Jones
rjones at redhat.com
Thu Jan 24 12:36:40 UTC 2013
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
More information about the Libguestfs
mailing list