Libguestfs - Mar 2015 - Re: [PATCH 1/2] parted: introduce enum for whether parted has option -m

Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
---
 daemon/parted.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index a7bcb99..64a7d3c 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -33,6 +33,12 @@ GUESTFSD_EXT_CMD(str_parted, parted);
 GUESTFSD_EXT_CMD(str_sfdisk, sfdisk);
 GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
 
+enum {
+  PARTED_INVALID = -1,
+  /* parted do not support -m option */
+  PARTED_OPT_NO_M,
+  PARTED_OPT_HAS_M};
+
 /* Notes:
  *
  * Parted 1.9 sends error messages to stdout, hence use of the
@@ -320,7 +326,7 @@ get_table_field (const char *line, int n)
 static int
 test_parted_m_opt (void)
 {
-  static int result = -1;
+  static int result = PARTED_INVALID;
 
   if (result >= 0)
     return result;
@@ -334,9 +340,9 @@ test_parted_m_opt (void)
   }
 
   if (err && strstr (err, "invalid option -- m"))
-    result = 0;
+    result = PARTED_OPT_NO_M;
   else
-    result = 1;
+    result = PARTED_OPT_HAS_M;
   return result;
 }
 
@@ -347,7 +353,7 @@ print_partition_table (const char *device, int
parted_has_m_opt)
   CLEANUP_FREE char *err = NULL;
   int r;
 
-  if (parted_has_m_opt)
+  if (PARTED_OPT_HAS_M == parted_has_m_opt)
     r = command (&out, &err, str_parted, "-m",
"--", device,
                  "unit", "b",
                  "print", NULL);
@@ -370,14 +376,14 @@ char *
 do_part_get_parttype (const char *device)
 {
   int parted_has_m_opt = test_parted_m_opt ();
-  if (parted_has_m_opt == -1)
+  if (parted_has_m_opt == PARTED_INVALID)
     return NULL;
 
   CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt);
   if (!out)
     return NULL;
 
-  if (parted_has_m_opt) {
+  if (PARTED_OPT_HAS_M == parted_has_m_opt) {
     /* New-style parsing using the "machine-readable" format from
      * 'parted -m'.
      */
@@ -452,7 +458,7 @@ 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)
+  if (parted_has_m_opt == PARTED_INVALID)
     return NULL;
 
   CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt);
@@ -466,7 +472,7 @@ do_part_list (const char *device)
 
   guestfs_int_partition_list *r;
 
-  if (parted_has_m_opt) {
+  if (PARTED_OPT_HAS_M == parted_has_m_opt) {
     /* New-style parsing using the "machine-readable" format from
      * 'parted -m'.
      *
@@ -578,7 +584,7 @@ do_part_get_bootable (const char *device, int partnum)
   }
 
   int parted_has_m_opt = test_parted_m_opt ();
-  if (parted_has_m_opt == -1)
+  if (parted_has_m_opt == PARTED_INVALID)
     return -1;
 
   CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt);
@@ -590,7 +596,7 @@ do_part_get_bootable (const char *device, int partnum)
   if (!lines)
     return -1;
 
-  if (parted_has_m_opt) {
+  if (PARTED_OPT_HAS_M == parted_has_m_opt) {
     /* New-style parsing using the "machine-readable" format from
      * 'parted -m'.
      *
@@ -965,14 +971,14 @@ do_part_get_name (const char *device, int partnum)
 
   if (STREQ (parttype, "gpt")) {
     int parted_has_m_opt = test_parted_m_opt ();
-    if (parted_has_m_opt == -1)
+    if (parted_has_m_opt == PARTED_INVALID)
       return NULL;
 
     CLEANUP_FREE char *out = print_partition_table (device, parted_has_m_opt);
     if (!out)
       return NULL;
 
-    if (parted_has_m_opt) {
+    if (PARTED_OPT_HAS_M == parted_has_m_opt) {
       /* New-style parsing using the "machine-readable" format from
        * 'parted -m'.
        */
-- 
2.1.0

On Tue, Mar 24, 2015 at 01:15:21PM +0100, Pino Toscano
wrote:
> On Tuesday 24 March 2015 07:20:16 Chen Hanxiao wrote:
> > Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
> > ---
> >  daemon/parted.c | 30 ++++++++++++++++++------------
> >  1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/daemon/parted.c b/daemon/parted.c
> > index a7bcb99..64a7d3c 100644
> > --- a/daemon/parted.c
> > +++ b/daemon/parted.c
> > @@ -33,6 +33,12 @@ GUESTFSD_EXT_CMD(str_parted, parted);
> >  GUESTFSD_EXT_CMD(str_sfdisk, sfdisk);
> >  GUESTFSD_EXT_CMD(str_sgdisk, sgdisk);
> >  
> > +enum {
> > +  PARTED_INVALID = -1,
> > +  /* parted do not support -m option */
> > +  PARTED_OPT_NO_M,
> > +  PARTED_OPT_HAS_M};
> 
> (I didn't have even the time to reply to the question about this enum)
> 
> PARTED_INVALID does not make much sense, especially that I was
> referring just to the parameter for print_partition_table, so for it
> only two values (eg PARTED_OUTPUT_MACHINE and PARTED_OUTPUT_NORMAL)
> are enough.
> 
> 
> > +
> >  /* Notes:
> >   *
> >   * Parted 1.9 sends error messages to stdout, hence use of the
> > @@ -320,7 +326,7 @@ get_table_field (const char *line, int n)
> >  static int
> >  test_parted_m_opt (void)
> >  {
> > -  static int result = -1;
> > +  static int result = PARTED_INVALID;
> 
> Commenting here, but it applies to the rest of the changes: if you want
> to apply an enum for this case, then do it consistently and for all the
> cases. Using an enum value and storing it to an int variable defeats
> the point of using an enum at all, as you will not catch non-enum
> values or not check to be handling all values where needed.
Agreed.

However I'm just going to make this fix and push it.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
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