Re: [ndctl patch 3/4] query_fw_finish_status: get rid of redundant variable

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Fri, 2019-10-18 at 17:06 -0400, Jeff Moyer wrote:
> Ira Weiny <ira.weiny@intel.com> writes:
> > On Fri, Oct 18, 2019 at 04:23:01PM -0400, Jeff Moyer wrote:
> > > The 'done' variable only adds confusion.
> > > 
> > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> > > ---
> > >  ndctl/dimm.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/ndctl/dimm.c b/ndctl/dimm.c
> > > index c8821d6..f28b9c1 100644
> > > --- a/ndctl/dimm.c
> > > +++ b/ndctl/dimm.c
> > > @@ -682,7 +682,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
> > >  	struct ndctl_cmd *cmd;
> > >  	int rc;
> > >  	enum ND_FW_STATUS status;
> > > -	bool done = false;
> > >  	struct timespec now, before, after;
> > >  	uint64_t ver;
> > >  
> > > @@ -716,7 +715,6 @@ static int query_fw_finish_status(struct ndctl_dimm *dimm,
> > >  					ndctl_dimm_get_devname(dimm));
> > >  			printf("Firmware version %#lx.\n", ver);
> > >  			printf("Cold reboot to activate.\n");
> > > -			done = true;
> > >  			rc = 0;
> > 
> > Do we need "goto out" here?
> 
> Yes, I missed that one.  Thanks.

This actually looks fine, since there is a 'break' down below.

> 
> > >  			break;
> > >  		case FW_EBUSY:

[..]

> > > -	} while (!done);
> > > +	} while (true);
> > 
> > I'm not a fan of "while (true)".  But I'm not the maintainer.  The Logic seems
> > fine otherwise.
> 
> The way things stand today is a mashup of goto vs. break.  I'll
> follow-up with fixed up patch next week if there is consensus on the
> change.  If you have a suggestion for a better way, that's welcome as
> well.
> 
I've applied this as is for v67, we can look at a refactoring for the
while (true) later.

_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org