Libguestfs - Mar 2015 - Re: [PATCH] part-list: add support for show partition type

We lack of showing parttition type for part-list command.
This patch will add support for this.
Also 'parted -m' did not provide partition type info,
remove code section for 'parted -m' process.

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
 daemon/parted.c      | 155 ++++++++++++++++++++++++++-------------------------
 generator/structs.ml |   1 +
 resize/resize.ml     |   2 +-
 3 files changed, 82 insertions(+), 76 deletions(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index a7bcb99..312b15f 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -33,6 +33,10 @@ GUESTFSD_EXT_CMD(str_parted, parted);
 GUESTFSD_EXT_CMD(str_sfdisk, sfdisk);
 GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
 
+#ifndef PARTED_NO_M
+# define PARTED_NO_M 0
+#endif
+
 /* Notes:
  *
  * Parted 1.9 sends error messages to stdout, hence use of the
@@ -451,11 +455,7 @@ do_part_get_parttype (const char *device)
 guestfs_int_partition_list *
 do_part_list (const char *device)
 {
-  int parted_has_m_opt = test_parted_m_opt ();
-  if (parted_has_m_opt == -1)
-    return NULL;
-
-  CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt);
+  CLEANUP_FREE char *out = print_partition_table (device, PARTED_NO_M);
   if (!out)
     return NULL;
 
@@ -466,88 +466,86 @@ do_part_list (const char *device)
 
   guestfs_int_partition_list *r;
 
-  if (parted_has_m_opt) {
-    /* New-style parsing using the "machine-readable" format from
-     * 'parted -m'.
-     *
-     * lines[0] is "BYT;", lines[1] is the device line which we
ignore,
-     * lines[2..] are the partitions themselves.  Count how many.
-     */
-    size_t nr_rows = 0, row;
-    for (row = 2; lines[row] != NULL; ++row)
-      ++nr_rows;
-
-    r = malloc (sizeof *r);
-    if (r == NULL) {
-      reply_with_perror ("malloc");
-      return NULL;
+  /* Old-style.  Start at the line following "^Number", up to the
+   * next blank line.
+   */
+  size_t start = 0, end = 0, has_type = 0, row;
+
+  for (row = 0; lines[row] != NULL; ++row)
+    if (STRPREFIX (lines[row], "Number")) {
+      start = row+1;
+    /* check whether output of parted has 'Type' field */
+    if (strstr (lines[row], "Type"))
+      has_type = 1;
+      break;
     }
-    r->guestfs_int_partition_list_len = nr_rows;
-    r->guestfs_int_partition_list_val -      malloc (nr_rows * sizeof
(guestfs_int_partition));
-    if (r->guestfs_int_partition_list_val == NULL) {
-      reply_with_perror ("malloc");
-      goto error2;
+
+  if (start == 0) {
+    reply_with_error ("parted output has no \"Number\"
line");
+    return NULL;
+  }
+
+  for (row = start; lines[row] != NULL; ++row)
+    if (STREQ (lines[row], "")) {
+      end = row;
+      break;
     }
 
-    /* Now parse the lines. */
-    size_t i;
-    for (i = 0, row = 2; lines[row] != NULL; ++i, ++row) {
-      if (sscanf (lines[row], "%d:%" SCNi64 "B:%" SCNi64
"B:%" SCNi64 "B",
+  if (end == 0) {
+    reply_with_error ("parted output has no blank after end of
table");
+    return NULL;
+  }
+
+  size_t nr_rows = end - start;
+
+  r = malloc (sizeof *r);
+  if (r == NULL) {
+    reply_with_perror ("malloc");
+    return NULL;
+  }
+  r->guestfs_int_partition_list_len = nr_rows;
+  r->guestfs_int_partition_list_val +    malloc (nr_rows * sizeof
(guestfs_int_partition));
+  if (r->guestfs_int_partition_list_val == NULL) {
+    reply_with_perror ("malloc");
+    goto error2;
+  }
+
+  /* Now parse the lines. */
+  size_t i, k;
+  if (has_type) {
+    /* hold type such as primary, logical and extended */
+    char type_temp[16];
+    for (i = 0, row = start; row < end; ++i, ++row) {
+      if (sscanf (lines[row], " %d %" SCNi64 "B %" SCNi64
"B %" SCNi64 "B" "%s",
                   &r->guestfs_int_partition_list_val[i].part_num,
                   &r->guestfs_int_partition_list_val[i].part_start,
                   &r->guestfs_int_partition_list_val[i].part_end,
-                  &r->guestfs_int_partition_list_val[i].part_size) != 4)
{
+                  &r->guestfs_int_partition_list_val[i].part_size,
+                  type_temp) != 5) {
         reply_with_error ("could not parse row from output of parted print
command: %s", lines[row]);
         goto error3;
       }
-    }
-  }
-  else {
-    /* Old-style.  Start at the line following "^Number", up to the
-     * next blank line.
-     */
-    size_t start = 0, end = 0, row;
 
-    for (row = 0; lines[row] != NULL; ++row)
-      if (STRPREFIX (lines[row], "Number")) {
-        start = row+1;
-        break;
+      if (STRPREFIX (type_temp, "primary")) {
+        r->guestfs_int_partition_list_val[i].part_type =
strdup("primary");
+        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
+          goto error4;
+      } else if (STRPREFIX (type_temp, "logical")) {
+        r->guestfs_int_partition_list_val[i].part_type =
strdup("logical");
+        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
+          goto error4;
+      } else if (STRPREFIX (type_temp, "extended")) {
+        r->guestfs_int_partition_list_val[i].part_type =
strdup("extended");
+        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
+          goto error4;
+      } else {
+        r->guestfs_int_partition_list_val[i].part_type =
strdup("");
+        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
+          goto error4;
       }
-
-    if (start == 0) {
-      reply_with_error ("parted output has no \"Number\"
line");
-      return NULL;
     }
-
-    for (row = start; lines[row] != NULL; ++row)
-      if (STREQ (lines[row], "")) {
-        end = row;
-        break;
-      }
-
-    if (end == 0) {
-      reply_with_error ("parted output has no blank after end of
table");
-      return NULL;
-    }
-
-    size_t nr_rows = end - start;
-
-    r = malloc (sizeof *r);
-    if (r == NULL) {
-      reply_with_perror ("malloc");
-      return NULL;
-    }
-    r->guestfs_int_partition_list_len = nr_rows;
-    r->guestfs_int_partition_list_val -      malloc (nr_rows * sizeof
(guestfs_int_partition));
-    if (r->guestfs_int_partition_list_val == NULL) {
-      reply_with_perror ("malloc");
-      goto error2;
-    }
-
-    /* Now parse the lines. */
-    size_t i;
+  } else {
     for (i = 0, row = start; row < end; ++i, ++row) {
       if (sscanf (lines[row], " %d %" SCNi64 "B %" SCNi64
"B %" SCNi64 "B",
                   &r->guestfs_int_partition_list_val[i].part_num,
@@ -557,11 +555,18 @@ do_part_list (const char *device)
         reply_with_error ("could not parse row from output of parted print
command: %s", lines[row]);
         goto error3;
       }
+
+      r->guestfs_int_partition_list_val[i].part_type = strdup("");
+      if (r->guestfs_int_partition_list_val[i].part_type == NULL)
+        goto error4;
     }
   }
 
   return r;
 
+ error4:
+ for (k = 0; k <= i; k++)
+  free (r->guestfs_int_partition_list_val[k].part_type);
  error3:
   free (r->guestfs_int_partition_list_val);
  error2:
diff --git a/generator/structs.ml b/generator/structs.ml
index ea110a1..e7a9fa6 100644
--- a/generator/structs.ml
+++ b/generator/structs.ml
@@ -240,6 +240,7 @@ let structs = [
     "part_start", FBytes;
     "part_end", FBytes;
     "part_size", FBytes;
+    "part_type", FString;
     ];
     s_camel_name = "Partition" };
 
diff --git a/resize/resize.ml b/resize/resize.ml
index 84fd6d4..8f8f67f 100644
--- a/resize/resize.ml
+++ b/resize/resize.ml
@@ -1078,7 +1078,7 @@ read the man page virt-resize(1).
            *)
           p_name = "";
           p_part = { G.part_num = 0l; part_start = 0L; part_end = 0L;
-                     part_size = 0L };
+                     part_size = 0L; part_type = "" };
           p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
           p_label = None; p_guid = None;
 
-- 
2.1.0

On Friday 13 March 2015 05:04:56 Chen Hanxiao wrote:
> We lack of showing parttition type for part-list command.
> This patch will add support for this.
You cannot extend the struct returned by part_init with new members,
that will cause compatibility issues with existing C users of that API.
> Also 'parted -m' did not provide partition type info,
> remove code section for 'parted -m' process.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> ---
>  daemon/parted.c      | 155
++++++++++++++++++++++++++-------------------------
>  generator/structs.ml |   1 +
>  resize/resize.ml     |   2 +-
>  3 files changed, 82 insertions(+), 76 deletions(-)
> 
> diff --git a/daemon/parted.c b/daemon/parted.c
> index a7bcb99..312b15f 100644
> --- a/daemon/parted.c
> +++ b/daemon/parted.c
> @@ -33,6 +33,10 @@ GUESTFSD_EXT_CMD(str_parted, parted);
>  GUESTFSD_EXT_CMD(str_sfdisk, sfdisk);
>  GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
>  
> +#ifndef PARTED_NO_M
> +# define PARTED_NO_M 0
> +#endif
> +
>  /* Notes:
>   *
>   * Parted 1.9 sends error messages to stdout, hence use of the
> @@ -451,11 +455,7 @@ do_part_get_parttype (const char *device)
>  guestfs_int_partition_list *
>  do_part_list (const char *device)
>  {
> -  int parted_has_m_opt = test_parted_m_opt ();
> -  if (parted_has_m_opt == -1)
> -    return NULL;
> -
> -  CLEANUP_FREE char *out = print_partition_table (device,
parted_has_m_opt);
> +  CLEANUP_FREE char *out = print_partition_table (device, PARTED_NO_M);
>    if (!out)
>      return NULL;
>  
> @@ -466,88 +466,86 @@ do_part_list (const char *device)
>  
>    guestfs_int_partition_list *r;
>  
> -  if (parted_has_m_opt) {
> -    /* New-style parsing using the "machine-readable" format
from
> -     * 'parted -m'.
> -     *
> -     * lines[0] is "BYT;", lines[1] is the device line which we
ignore,
> -     * lines[2..] are the partitions themselves.  Count how many.
> -     */
> -    size_t nr_rows = 0, row;
> -    for (row = 2; lines[row] != NULL; ++row)
> -      ++nr_rows;
> -
> -    r = malloc (sizeof *r);
> -    if (r == NULL) {
> -      reply_with_perror ("malloc");
> -      return NULL;
> +  /* Old-style.  Start at the line following "^Number", up to
the
> +   * next blank line.
> +   */
> +  size_t start = 0, end = 0, has_type = 0, row;
> +
> +  for (row = 0; lines[row] != NULL; ++row)
> +    if (STRPREFIX (lines[row], "Number")) {
> +      start = row+1;
> +    /* check whether output of parted has 'Type' field */
> +    if (strstr (lines[row], "Type"))
> +      has_type = 1;
> +      break;
>      }
> -    r->guestfs_int_partition_list_len = nr_rows;
> -    r->guestfs_int_partition_list_val > -      malloc (nr_rows *
sizeof (guestfs_int_partition));
> -    if (r->guestfs_int_partition_list_val == NULL) {
> -      reply_with_perror ("malloc");
> -      goto error2;
> +
> +  if (start == 0) {
> +    reply_with_error ("parted output has no \"Number\"
line");
> +    return NULL;
> +  }
> +
> +  for (row = start; lines[row] != NULL; ++row)
> +    if (STREQ (lines[row], "")) {
> +      end = row;
> +      break;
>      }
>  
> -    /* Now parse the lines. */
> -    size_t i;
> -    for (i = 0, row = 2; lines[row] != NULL; ++i, ++row) {
> -      if (sscanf (lines[row], "%d:%" SCNi64 "B:%"
SCNi64 "B:%" SCNi64 "B",
> +  if (end == 0) {
> +    reply_with_error ("parted output has no blank after end of
table");
> +    return NULL;
> +  }
> +
> +  size_t nr_rows = end - start;
> +
> +  r = malloc (sizeof *r);
> +  if (r == NULL) {
> +    reply_with_perror ("malloc");
> +    return NULL;
> +  }
> +  r->guestfs_int_partition_list_len = nr_rows;
> +  r->guestfs_int_partition_list_val > +    malloc (nr_rows * sizeof
(guestfs_int_partition));
> +  if (r->guestfs_int_partition_list_val == NULL) {
> +    reply_with_perror ("malloc");
> +    goto error2;
> +  }
> +
> +  /* Now parse the lines. */
> +  size_t i, k;
> +  if (has_type) {
> +    /* hold type such as primary, logical and extended */
> +    char type_temp[16];
> +    for (i = 0, row = start; row < end; ++i, ++row) {
> +      if (sscanf (lines[row], " %d %" SCNi64 "B %"
SCNi64 "B %" SCNi64 "B" "%s",
>                    &r->guestfs_int_partition_list_val[i].part_num,
>                    &r->guestfs_int_partition_list_val[i].part_start,
>                    &r->guestfs_int_partition_list_val[i].part_end,
> -                  &r->guestfs_int_partition_list_val[i].part_size)
!= 4) {
> +                  &r->guestfs_int_partition_list_val[i].part_size,
> +                  type_temp) != 5) {
>          reply_with_error ("could not parse row from output of parted
print command: %s", lines[row]);
>          goto error3;
>        }
> -    }
> -  }
> -  else {
> -    /* Old-style.  Start at the line following "^Number", up to
the
> -     * next blank line.
> -     */
> -    size_t start = 0, end = 0, row;
>  
> -    for (row = 0; lines[row] != NULL; ++row)
> -      if (STRPREFIX (lines[row], "Number")) {
> -        start = row+1;
> -        break;
> +      if (STRPREFIX (type_temp, "primary")) {
> +        r->guestfs_int_partition_list_val[i].part_type =
strdup("primary");
> +        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
> +          goto error4;
> +      } else if (STRPREFIX (type_temp, "logical")) {
> +        r->guestfs_int_partition_list_val[i].part_type =
strdup("logical");
> +        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
> +          goto error4;
> +      } else if (STRPREFIX (type_temp, "extended")) {
> +        r->guestfs_int_partition_list_val[i].part_type =
strdup("extended");
> +        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
> +          goto error4;
> +      } else {
> +        r->guestfs_int_partition_list_val[i].part_type =
strdup("");
> +        if (r->guestfs_int_partition_list_val[i].part_type == NULL)
> +          goto error4;
>        }
> -
> -    if (start == 0) {
> -      reply_with_error ("parted output has no \"Number\"
line");
> -      return NULL;
>      }
> -
> -    for (row = start; lines[row] != NULL; ++row)
> -      if (STREQ (lines[row], "")) {
> -        end = row;
> -        break;
> -      }
> -
> -    if (end == 0) {
> -      reply_with_error ("parted output has no blank after end of
table");
> -      return NULL;
> -    }
> -
> -    size_t nr_rows = end - start;
> -
> -    r = malloc (sizeof *r);
> -    if (r == NULL) {
> -      reply_with_perror ("malloc");
> -      return NULL;
> -    }
> -    r->guestfs_int_partition_list_len = nr_rows;
> -    r->guestfs_int_partition_list_val > -      malloc (nr_rows *
sizeof (guestfs_int_partition));
> -    if (r->guestfs_int_partition_list_val == NULL) {
> -      reply_with_perror ("malloc");
> -      goto error2;
> -    }
> -
> -    /* Now parse the lines. */
> -    size_t i;
> +  } else {
>      for (i = 0, row = start; row < end; ++i, ++row) {
>        if (sscanf (lines[row], " %d %" SCNi64 "B %"
SCNi64 "B %" SCNi64 "B",
>                    &r->guestfs_int_partition_list_val[i].part_num,
> @@ -557,11 +555,18 @@ do_part_list (const char *device)
>          reply_with_error ("could not parse row from output of parted
print command: %s", lines[row]);
>          goto error3;
>        }
> +
> +      r->guestfs_int_partition_list_val[i].part_type =
strdup("");
> +      if (r->guestfs_int_partition_list_val[i].part_type == NULL)
> +        goto error4;
>      }
>    }
>  
>    return r;
>  
> + error4:
> + for (k = 0; k <= i; k++)
> +  free (r->guestfs_int_partition_list_val[k].part_type);
>   error3:
>    free (r->guestfs_int_partition_list_val);
>   error2:
> diff --git a/generator/structs.ml b/generator/structs.ml
> index ea110a1..e7a9fa6 100644
> --- a/generator/structs.ml
> +++ b/generator/structs.ml
> @@ -240,6 +240,7 @@ let structs = [
>      "part_start", FBytes;
>      "part_end", FBytes;
>      "part_size", FBytes;
> +    "part_type", FString;
>      ];
>      s_camel_name = "Partition" };
>  
I'd just add a new part_get_type (or something similar), which would
call parted to return "primary"/etc. This would match what we do
already with other attributes of partitions, and not force part_list
to use a non-machine output type (which is not really meant for
machine parsing).
> diff --git a/resize/resize.ml b/resize/resize.ml
> index 84fd6d4..8f8f67f 100644
> --- a/resize/resize.ml
> +++ b/resize/resize.ml
> @@ -1078,7 +1078,7 @@ read the man page virt-resize(1).
>             *)
>            p_name = "";
>            p_part = { G.part_num = 0l; part_start = 0L; part_end = 0L;
> -                     part_size = 0L };
> +                     part_size = 0L; part_type = "" };
>            p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
>            p_label = None; p_guid = None;
Better add this information directly in the type partition, possibly
also making use of type partition_type.

Also this could go in a different patch than the above changes to get
the primary/extended/etc type of a partition.

Thanks,
-- 
Pino Toscano

> -----Original Message-----
> From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com]
On
> Behalf Of Pino Toscano
> Sent: Friday, March 13, 2015 6:32 PM
> To: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH] part-list: add support for show partition
type
> 
> On Friday 13 March 2015 05:04:56 Chen Hanxiao wrote:
> > We lack of showing parttition type for part-list command.
> > This patch will add support for this.
> 
> You cannot extend the struct returned by part_init with new members,
> that will cause compatibility issues with existing C users of that API.
> 
> > Also 'parted -m' did not provide partition type info,
> > remove code section for 'parted -m' process.
> >
> > diff --git a/generator/structs.ml b/generator/structs.ml
> > index ea110a1..e7a9fa6 100644
> > --- a/generator/structs.ml
> > +++ b/generator/structs.ml
> > @@ -240,6 +240,7 @@ let structs = [
> >      "part_start", FBytes;
> >      "part_end", FBytes;
> >      "part_size", FBytes;
> > +    "part_type", FString;
> >      ];
> >      s_camel_name = "Partition" };
> >
> 
> I'd just add a new part_get_type (or something similar), which would
> call parted to return "primary"/etc. This would match what we do
> already with other attributes of partitions, and not force part_list
> to use a non-machine output type (which is not really meant for
> machine parsing).
Alright.
A new API would keep compatibility safe.
> 
> > diff --git a/resize/resize.ml b/resize/resize.ml
> > index 84fd6d4..8f8f67f 100644
> > --- a/resize/resize.ml
> > +++ b/resize/resize.ml
> > @@ -1078,7 +1078,7 @@ read the man page virt-resize(1).
> >             *)
> >            p_name = "";
> >            p_part = { G.part_num = 0l; part_start = 0L; part_end = 0L;
> > -                     part_size = 0L };
> > +                     part_size = 0L; part_type = "" };
> >            p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
> >            p_label = None; p_guid = None;
> 
> Better add this information directly in the type partition, possibly
> also making use of type partition_type.
> 
> Also this could go in a different patch than the above changes to get
> the primary/extended/etc type of a partition.
I'll send a different patch for virt-resize on this.

Regards,
- Chen