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

[Chen, Hanxiao]

> -----Original Message-----
> From: libguestfs-bounces at redhat.com [mailto:libguestfs-bounces at redhat.com] On
> Behalf Of Richard W.M. Jones
> Sent: Tuesday, March 24, 2015 9:31 PM
> To: Pino Toscano
> Cc: libguestfs at redhat.com
> Subject: Re: [Libguestfs] [PATCH 1/2] parted: introduce enum for whether parted
> has option -m
> 
> 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 at 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.
> 

Hi Rich, Pino

Thanks for your help.

Regards,
- Chen