Libguestfs - Mar 2015 - Re: [PATCH] New API: part_get_part_type for showing partition type

> -----Original Message-----
> From: Richard W.M. Jones [mailto:rjones@redhat.com]
> Sent: Monday, March 23, 2015 9:29 PM
> To: Chen, Hanxiao/陈 晗霄
> Cc: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH] New API: part_get_part_type for showing
partition
> type
> 
> On Tue, Mar 17, 2015 at 02:45:46AM -0400, Chen Hanxiao wrote:
> > This patch will add support for getting partition type
> > of a partiton numbered device.
> >
> > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> > ---
> >  daemon/parted.c      | 112
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  generator/actions.ml |  18 +++++++++
> >  src/MAX_PROC_NR      |   2 +-
> >  3 files changed, 131 insertions(+), 1 deletion(-)
> >
> > diff --git a/daemon/parted.c b/daemon/parted.c
> > index a7bcb99..0ae6e5c 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
> 
> What does this bit do?
I want to use print_partition_table(xxx, 0)
but do not want to pass a raw '0'.
Maybe the name is not so clear.
I'll rename it to PARTED_NOT_USE_M in the next version.
> 
> >  /* Notes:
> >   *
> >   * Parted 1.9 sends error messages to stdout, hence use of the
> > @@ -1022,3 +1026,111 @@ do_part_get_name (const char *device, int
partnum)
> >    reply_with_error ("cannot get the partition name from
'%s' layouts", parttype);
> >    return NULL;
> >  }
> > +
> > +char *
> > +do_part_get_part_type (const char *device, int partnum)
> > +{
> > +  CLEANUP_FREE char *parttype;
> > +  char *part_type;
> > +
> > +  if (partnum <= 0) {
> > +    reply_with_error ("partition number must be >= 1");
> > +    return NULL;
> > +  }
> > +
> > +  parttype = do_part_get_parttype (device);
> > +  if (parttype == NULL)
> > +    return NULL;
> > +
> > +  if (STREQ (parttype, "gpt")) {
> > +    part_type = strdup("primary");
> > +    if (part_type == NULL) {
> > +      reply_with_error ("strdup failed");
> > +      return NULL;
> > +    }
> > +    return part_type;
> > +  }
> > +
> > +  /* machine parseable output by 'parted -m' did not provide
> > +   * partition type info.
> > +   * Use traditional style.
> > +   */
> > +  CLEANUP_FREE char *out = print_partition_table (device,
PARTED_NO_M);
> > +  if (!out)
> > +    return NULL;
> > +
> > +  CLEANUP_FREE_STRING_LIST char **lines = split_lines (out);
> > +
> > +  if (!lines)
> > +    return NULL;
> > +
> > +  size_t start = 0, end = 0, row;
> > +
> > +  for (row = 0; lines[row] != NULL; ++row)
> > +    if (STRPREFIX (lines[row], "Number")) {
> > +      start = row + 1;
> > +      break;
> > +    }
> > +
> > +  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;
> > +  }
> > +
> > +  /* Now parse the lines. */
> > +  size_t i;
> > +  int64_t temp_int64;
> > +  int part_num;
> > +  char temp_type[16];
> > +  for (i = 0, row = start;  row < end; ++i, ++row) {
> > +    if (sscanf (lines[row], "%d%" SCNi64 "B%"
SCNi64 "B%" SCNi64 "B" "%s",
> > +                &part_num,
> > +                &temp_int64,
> > +                &temp_int64,
> > +                &temp_int64,
> > +                temp_type) != 5) {
> 
> We're hoping here that the output of 'parted' never changes and
> overflows the 16 byte buffer.
> 
> Instead of hoping, you can guarantee that by replacing %s with %15s
Will fix.

Regards,
- Chen
> 
> > +      reply_with_error ("could not parse row from output of
parted print
> command: %s", lines[row]);
> > +      return NULL;
> > +    }
> > +
> > +    if (part_num != partnum)
> > +        continue;
> > +
> > +    if (STRPREFIX (temp_type, "primary")) {
> > +      part_type = strdup("primary");
> > +      if (part_type == NULL)
> > +          goto error;
> > +    } else if (STRPREFIX (temp_type, "logical")) {
> > +      part_type = strdup("logical");
> > +      if (part_type == NULL)
> > +          goto error;
> > +    } else if (STRPREFIX (temp_type, "extended")) {
> > +      part_type = strdup("extended");
> > +      if (part_type == NULL)
> > +          goto error;
> > +    } else
> > +        goto error;
> > +
> > +    return part_type;
> > +  }
> > +
> > +  if (row == end) {
> > +    reply_with_error ("could not find partnum: %d",
partnum);
> > +    return NULL;
> > +  }
> > +
> > +  error:
> > +    reply_with_error ("strdup failed");
> > +    return NULL;
> > +}
> > diff --git a/generator/actions.ml b/generator/actions.ml
> > index fb971d3..72418b0 100644
> > --- a/generator/actions.ml
> > +++ b/generator/actions.ml
> > @@ -12522,6 +12522,24 @@ This will Enable extended inode refs."
};
> >      longdesc = "\
> >  This enable skinny metadata extent refs." };
> >
> > +  { defaults with
> > +    name = "part_get_part_type";
> > +    style = RString "partitiontype", [Device
"device"; Int "partnum"], [];
> > +    proc_nr = Some 453;
> > +    tests = [
> > +      InitEmpty, Always, TestResultString (
> > +        [["part_init"; "/dev/sda";
"mbr"];
> > +         ["part_add"; "/dev/sda"; "p";
"64"; "204799"];
> > +         ["part_add"; "/dev/sda"; "e";
"204800"; "614400"];
> > +         ["part_add"; "/dev/sda"; "l";
"204864"; "205988"];
> > +         ["part_get_part_type"; "/dev/sda";
"5"]], "logical"), []
> > +    ];
> > +
> > +    shortdesc = "get the partition type";
> > +    longdesc = "\
> > +This could get the partition type, such as primary, logical,
> > +on partition numbered C<partnum> on device
C<device>." };
> > +
> >  ]
> >
> >  (* Non-API meta-commands available only in guestfish.
> > diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
> > index 8670c73..534b992 100644
> > --- a/src/MAX_PROC_NR
> > +++ b/src/MAX_PROC_NR
> > @@ -1 +1 @@
> > -452
> > +453
> 
> Rich.
> 
> --
> Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://people.redhat.com/~rjones/virt-top

On Tuesday 24 March 2015 05:25:51 Chen, Hanxiao wrote:
> 
> > -----Original Message-----
> > From: Richard W.M. Jones [mailto:rjones@redhat.com]
> > Sent: Monday, March 23, 2015 9:29 PM
> > To: Chen, Hanxiao/陈 晗霄
> > Cc: libguestfs@redhat.com
> > Subject: Re: [Libguestfs] [PATCH] New API: part_get_part_type for
showing partition
> > type
> > 
> > On Tue, Mar 17, 2015 at 02:45:46AM -0400, Chen Hanxiao wrote:
> > > This patch will add support for getting partition type
> > > of a partiton numbered device.
> > >
> > > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> > > ---
> > >  daemon/parted.c      | 112
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  generator/actions.ml |  18 +++++++++
> > >  src/MAX_PROC_NR      |   2 +-
> > >  3 files changed, 131 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/daemon/parted.c b/daemon/parted.c
> > > index a7bcb99..0ae6e5c 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
> > 
> > What does this bit do?
> 
> I want to use print_partition_table(xxx, 0)
> but do not want to pass a raw '0'.
> Maybe the name is not so clear.
> I'll rename it to PARTED_NOT_USE_M in the next version.
Improving the readability of a raw integer is a good thing; however,
I'd suggest switching to an enum instead.

-- 
Pino Toscano

> -----Original Message-----
> From: libguestfs-bounces@redhat.com [mailto:libguestfs-bounces@redhat.com]
On
> Behalf Of Pino Toscano
> Sent: Tuesday, March 24, 2015 4:44 PM
> To: libguestfs@redhat.com
> Subject: Re: [Libguestfs] [PATCH] New API: part_get_part_type for showing
partition
> type
> 
> On Tuesday 24 March 2015 05:25:51 Chen, Hanxiao wrote:
> >
> > > -----Original Message-----
> > > From: Richard W.M. Jones [mailto:rjones@redhat.com]
> > > Sent: Monday, March 23, 2015 9:29 PM
> > > To: Chen, Hanxiao/陈 晗霄
> > > Cc: libguestfs@redhat.com
> > > Subject: Re: [Libguestfs] [PATCH] New API: part_get_part_type for
showing
> partition
> > > type
> > >
> > > On Tue, Mar 17, 2015 at 02:45:46AM -0400, Chen Hanxiao wrote:
> > > > This patch will add support for getting partition type
> > > > of a partiton numbered device.
> > > >
> > > > Signed-off-by: Chen Hanxiao
<chenhanxiao@cn.fujitsu.com>
> > > > ---
> > > >  daemon/parted.c      | 112
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  generator/actions.ml |  18 +++++++++
> > > >  src/MAX_PROC_NR      |   2 +-
> > > >  3 files changed, 131 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/daemon/parted.c b/daemon/parted.c
> > > > index a7bcb99..0ae6e5c 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
> > >
> > > What does this bit do?
> >
> > I want to use print_partition_table(xxx, 0)
> > but do not want to pass a raw '0'.
> > Maybe the name is not so clear.
> > I'll rename it to PARTED_NOT_USE_M in the next version.
> 
> Improving the readability of a raw integer is a good thing; however,
> I'd suggest switching to an enum instead.
> 
Hi，

How about:
enum {
  PARTED_INVALID = -1,
  /* parted do not support -m option */
  PARTED_OPT_NO_M,
  PARTED_OPT_M};

print_partition_table(xxx, PARTED_OPT_NO_M)

Regards,
- Chen