Libguestfs - Feb 2014 - Re: [PATCH 2/3] New API: part-get-name (RHBZ#593511).

Hi,

attached there are few patches to implement a way to get the label of
GPT partitions (refactoring an existing function and adding a new
daemon API) and using it in virt-resize to restore them when copying
partitions.

Thanks,

Pino Toscano (3):
  daemon: parted: refactor sgdisk info parsing code
  New API: part-get-name (RHBZ#593511).
  resize: preserve GPT partition names (RHBZ#1060404).

 daemon/parted.c      | 86 ++++++++++++++++++++++++++++++++++++++++++----------
 generator/actions.ml | 13 ++++++++
 resize/resize.ml     | 19 +++++++++++-
 src/MAX_PROC_NR      |  2 +-
 4 files changed, 102 insertions(+), 18 deletions(-)

-- 
1.8.3.1

Isolate in an own function the code that runs sgdisk and parse a field
of it (using an extraction function passed as parameter), using it for
the GUID type.

This is just code motion, no actual behaviour changes.
---
 daemon/parted.c | 53 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index bd81986..5282adb 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -784,8 +784,9 @@ do_part_set_gpt_type(const char *device, int partnum, const
char *guid)
   return 0;
 }
 
-char *
-do_part_get_gpt_type(const char *device, int partnum)
+static char *
+sgdisk_info_extract_field (const char *device, int partnum, const char *field,
+                           char *(*extract) (const char *path))
 {
   if (partnum <= 0) {
     reply_with_error ("partition number must be >= 1");
@@ -814,6 +815,8 @@ do_part_get_gpt_type(const char *device, int partnum)
     return NULL;
   }
 
+  int fieldlen = strlen (field);
+
   /* Parse the output of sgdisk -i:
    * Partition GUID code: 21686148-6449-6E6F-744E-656564454649 (BIOS boot
partition)
    * Partition unique GUID: 19AEC5FE-D63A-4A15-9D37-6FCBFB873DC0
@@ -832,28 +835,22 @@ do_part_get_gpt_type(const char *device, int partnum)
     /* Split the line in 2 at the colon */
     char *colon = strchr (line, ':');
     if (colon) {
-#define SEARCH "Partition GUID code"
-      if (colon - line == strlen(SEARCH) &&
-          memcmp (line, SEARCH, strlen(SEARCH)) == 0)
+      if (colon - line == fieldlen &&
+          memcmp (line, field, fieldlen) == 0)
       {
-#undef SEARCH
         /* The value starts after the colon */
         char *value = colon + 1;
 
         /* Skip any leading whitespace */
         value += strspn (value, " \t");
 
-        /* The value contains only valid GUID characters */
-        size_t value_len = strspn (value, "-0123456789ABCDEF");
-
-        char *ret = malloc (value_len + 1);
+        /* Extract the actual information from the field. */
+        char *ret = extract (value);
         if (ret == NULL) {
-          reply_with_perror ("malloc");
+          /* The extraction function already sends the error. */
           return NULL;
         }
 
-        memcpy (ret, value, value_len);
-        ret[value_len] = '\0';
         return ret;
       }
     } else {
@@ -866,8 +863,32 @@ do_part_get_gpt_type(const char *device, int partnum)
     }
   }
 
-  /* If we got here it means we didn't find the Partition GUID code */
-  reply_with_error ("sgdisk output did not contain Partition GUID code.
"
-                    "See LIBGUESTFS_DEBUG output for more details");
+  /* If we got here it means we didn't find the field */
+  reply_with_error ("sgdisk output did not contain '%s'. "
+                    "See LIBGUESTFS_DEBUG output for more details",
field);
   return NULL;
 }
+
+static char *
+extract_uuid (const char *value)
+{
+  /* The value contains only valid GUID characters */
+  size_t value_len = strspn (value, "-0123456789ABCDEF");
+
+  char *ret = malloc (value_len + 1);
+  if (ret == NULL) {
+    reply_with_perror ("malloc");
+    return NULL;
+  }
+
+  memcpy (ret, value, value_len);
+  ret[value_len] = '\0';
+  return ret;
+}
+
+char *
+do_part_get_gpt_type (const char *device, int partnum)
+{
+  return sgdisk_info_extract_field (device, partnum,
+                                    "Partition GUID code",
extract_uuid);
+}
-- 
1.8.3.1

Counterpart of part-set-name, it uses sgdisk (hence needs the "gdisk"
feature) to query for the label/name of a partition in a GPT layout.
---
 daemon/parted.c      | 33 +++++++++++++++++++++++++++++++++
 generator/actions.ml | 13 +++++++++++++
 src/MAX_PROC_NR      |  2 +-
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index 5282adb..83f2411 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -886,9 +886,42 @@ extract_uuid (const char *value)
   return ret;
 }
 
+static char *
+extract_optionally_quoted (const char *value)
+{
+  size_t value_len = strlen (value);
+
+  if (value_len >= 2 &&
+      ((value[0] == '\'' && value[value_len - 1] ==
'\'') ||
+       (value[0] == '"' && value[value_len - 1] ==
'"'))) {
+    value_len -= 2;
+    ++value;
+  }
+
+  char *ret = strndup (value, value_len);
+  if (ret == NULL) {
+    reply_with_perror ("strndup");
+    return NULL;
+  }
+
+  return ret;
+}
+
 char *
 do_part_get_gpt_type (const char *device, int partnum)
 {
   return sgdisk_info_extract_field (device, partnum,
                                     "Partition GUID code",
extract_uuid);
 }
+
+char *
+do_part_get_name (const char *device, int partnum)
+{
+  char *parttype = do_part_get_parttype (device);
+  if (STREQ (parttype, "gpt"))
+    return sgdisk_info_extract_field (device, partnum,
+                                      "Partition name",
extract_optionally_quoted);
+
+  reply_with_error ("cannot get the partition name from '%s'
layouts", parttype);
+  return NULL;
+}
diff --git a/generator/actions.ml b/generator/actions.ml
index 176de98..b665149 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -11766,6 +11766,19 @@ enables all the other flags, if they are not specified
already.
 
 =back" };
 
+  { defaults with
+    name = "part_get_name";
+    style = RString "name", [Device "device"; Int
"partnum"], [];
+    proc_nr = Some 416;
+    optional = Some "gdisk";
+    shortdesc = "get partition name";
+    longdesc = "\
+This gets the partition name on partition numbered C<partnum> on
+device C<device>.  Note that partitions are numbered from 1.
+
+The partition name can only be read on certain types of partition
+table.  This works on C<gpt> but not on C<mbr> partitions." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 21c8d99..1c105f1 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-415
+416
-- 
1.8.3.1

Save the partition names/labels of the source partitions, and restore
them after the partition copy.
---
 resize/resize.ml | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/resize/resize.ml b/resize/resize.ml
index 191be83..c1794ed 100644
--- a/resize/resize.ml
+++ b/resize/resize.ml
@@ -49,6 +49,7 @@ type partition = {
   p_bootable : bool;             (* Is it bootable? *)
   p_id : partition_id;           (* Partition (MBR/GPT) ID. *)
   p_type : partition_content;    (* Content type and content size. *)
+  p_label : string option;       (* Label/name. *)
 
   (* What we're going to do: *)
   mutable p_operation : partition_operation;
@@ -84,7 +85,12 @@ let rec debug_partition p      | MBR_ID i -> sprintf
"0x%x" i
     | GPT_Type i -> i
     );
-  eprintf "\tcontent: %s\n" (string_of_partition_content p.p_type)
+  eprintf "\tcontent: %s\n" (string_of_partition_content p.p_type);
+  eprintf "\tlabel: %s\n"
+    (match p.p_label with
+    | Some label -> label
+    | None -> "(none)"
+    )
 and string_of_partition_content = function
   | ContentUnknown -> "unknown data"
   | ContentPV sz -> sprintf "LVM PV (%Ld bytes)" sz
@@ -459,9 +465,13 @@ read the man page virt-resize(1).
           let typ              if is_extended_partition id then
ContentExtendedPartition
             else get_partition_content name in
+          let label +            try Some (g#part_get_name "/dev/sda"
part_num)
+            with G.Error _ -> None in
 
           { p_name = name; p_part = part;
             p_bootable = bootable; p_id = id; p_type = typ;
+            p_label = label;
             p_operation = OpCopy; p_target_partnum = 0;
             p_target_start = 0L; p_target_end = 0L }
       ) parts in
@@ -1040,6 +1050,7 @@ read the man page virt-resize(1).
             p_part = { G.part_num = 0l; part_start = 0L; part_end = 0L;
                        part_size = 0L };
             p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
+            p_label = None;
 
             (* Target information is meaningful. *)
             p_operation = OpIgnore;
@@ -1117,6 +1128,12 @@ read the man page virt-resize(1).
       if p.p_bootable then
         g#part_set_bootable "/dev/sdb" p.p_target_partnum true;
 
+      (match p.p_label with
+      | Some label ->
+        g#part_set_name "/dev/sdb" p.p_target_partnum label;
+      | None -> ()
+      );
+
       match parttype, p.p_id with
       | GPT, GPT_Type gpt_type ->
         g#part_set_gpt_type "/dev/sdb" p.p_target_partnum gpt_type
-- 
1.8.3.1

On Tue, Feb 04, 2014 at 04:01:31PM +0100, Pino Toscano
wrote:
> @@ -832,28 +835,22 @@ do_part_get_gpt_type(const char *device, int partnum)
>      /* Split the line in 2 at the colon */
>      char *colon = strchr (line, ':');
>      if (colon) {
> -#define SEARCH "Partition GUID code"
> -      if (colon - line == strlen(SEARCH) &&
> -          memcmp (line, SEARCH, strlen(SEARCH)) == 0)
> +      if (colon - line == fieldlen &&
> +          memcmp (line, field, fieldlen) == 0)
Maybe use STRPREFIX macro here?

Looks fine, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tue, Feb 04, 2014 at 04:01:32PM +0100, Pino Toscano
wrote:
> +static char *
> +extract_optionally_quoted (const char *value)
> +{
> +  size_t value_len = strlen (value);
> +
> +  if (value_len >= 2 &&
> +      ((value[0] == '\'' && value[value_len - 1] ==
'\'') ||
> +       (value[0] == '"' && value[value_len - 1] ==
'"'))) {
> +    value_len -= 2;
> +    ++value;
> +  }
> +
> +  char *ret = strndup (value, value_len);
> +  if (ret == NULL) {
> +    reply_with_perror ("strndup");
> +    return NULL;
> +  }
> +
> +  return ret;
> +}
My spidey sense asks what happens if the value contains quote
characters?  I wonder if sgdisk escapes them.

Rest of the patch looks fine, but I think you should try setting the
partition name to a few weird values and see what sgdisk does.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tue, Feb 04, 2014 at 04:01:33PM +0100, Pino Toscano
wrote:
> Save the partition names/labels of the source partitions, and restore
> them after the partition copy.
> ---
>  resize/resize.ml | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/resize/resize.ml b/resize/resize.ml
> index 191be83..c1794ed 100644
> --- a/resize/resize.ml
> +++ b/resize/resize.ml
> @@ -49,6 +49,7 @@ type partition = {
>    p_bootable : bool;             (* Is it bootable? *)
>    p_id : partition_id;           (* Partition (MBR/GPT) ID. *)
>    p_type : partition_content;    (* Content type and content size. *)
> +  p_label : string option;       (* Label/name. *)
>  
>    (* What we're going to do: *)
>    mutable p_operation : partition_operation;
> @@ -84,7 +85,12 @@ let rec debug_partition p >      | MBR_ID i ->
sprintf "0x%x" i
>      | GPT_Type i -> i
>      );
> -  eprintf "\tcontent: %s\n" (string_of_partition_content
p.p_type)
> +  eprintf "\tcontent: %s\n" (string_of_partition_content
p.p_type);
> +  eprintf "\tlabel: %s\n"
> +    (match p.p_label with
> +    | Some label -> label
> +    | None -> "(none)"
> +    )
>  and string_of_partition_content = function
>    | ContentUnknown -> "unknown data"
>    | ContentPV sz -> sprintf "LVM PV (%Ld bytes)" sz
> @@ -459,9 +465,13 @@ read the man page virt-resize(1).
>            let typ >              if is_extended_partition id then
ContentExtendedPartition
>              else get_partition_content name in
> +          let label > +            try Some (g#part_get_name
"/dev/sda" part_num)
> +            with G.Error _ -> None in
>  
>            { p_name = name; p_part = part;
>              p_bootable = bootable; p_id = id; p_type = typ;
> +            p_label = label;
>              p_operation = OpCopy; p_target_partnum = 0;
>              p_target_start = 0L; p_target_end = 0L }
>        ) parts in
> @@ -1040,6 +1050,7 @@ read the man page virt-resize(1).
>              p_part = { G.part_num = 0l; part_start = 0L; part_end = 0L;
>                         part_size = 0L };
>              p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
> +            p_label = None;
>  
>              (* Target information is meaningful. *)
>              p_operation = OpIgnore;
> @@ -1117,6 +1128,12 @@ read the man page virt-resize(1).
>        if p.p_bootable then
>          g#part_set_bootable "/dev/sdb" p.p_target_partnum true;
>  
> +      (match p.p_label with
> +      | Some label ->
> +        g#part_set_name "/dev/sdb" p.p_target_partnum label;
> +      | None -> ()
> +      );
> +
>        match parttype, p.p_id with
>        | GPT, GPT_Type gpt_type ->
>          g#part_set_gpt_type "/dev/sdb" p.p_target_partnum
gpt_type
> -- 
> 1.8.3.1
ACK.

Rich.

-- 
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 KVM guests.
http://libguestfs.org/virt-v2v