Re: [ndctl PATCH v4] libndctl: Update nvdimm flags in ndctl_cmd_submit()

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

On Sat, 2022-02-19 at 18:09 +0530, Vaibhav Jain wrote:
> 
> > > @@ -1924,9 +1940,13 @@ static void *add_dimm(void *parent, int id, const char *dimm_base)
> > >  	dimm->formats = formats;
> > >  	/* Check if the given dimm supports nfit */
> > >  	if (ndctl_bus_has_nfit(bus)) {
> > > -		rc = populate_dimm_attributes(dimm, dimm_base, "nfit");
> > > +		dimm->bus_prefix = strdup("nfit");
> > > +		rc = dimm->bus_prefix ?
> > > +			populate_dimm_attributes(dimm, dimm_base) : -ENOMEM
> > >  	} else if (ndctl_bus_has_of_node(bus)) {
> > > -		rc = add_papr_dimm(dimm, dimm_base);
> > > +		dimm->bus_prefix = strdup("papr");
> > > +		rc = dimm->bus_prefix ?
> > > +			add_papr_dimm(dimm, dimm_base) : -ENOMEM;
> > 
> > For both of the above, it would be a bit more readable to just return
> > ENOMEM directly after strdup() if it fails, and then carry on with
> > add_<foo>_dimm().
> > 
> > 	dimm->bus_prefix = strdup("papr");
> > 	if (!dimm->bus_prefix)
> > 		return -ENOMEM;
> > 	rc = add_papr_dimm(dimm, dimm_base);
> > 	...
> > 
> Agree on the readability part but returning from there right away would
> prevent the allocated 'struct ndctl_dimm *dimm' from being freed in the
> error path. Also the function add_dimm() returns a 'void *' to 'struct
> ndctl_dimm*' right now rather than an 'int'.
> 
> I propose updating the code as:
> 
> 	if (ndctl_bus_has_nfit(bus)) {
> 		dimm->bus_prefix = strdup("nfit");
> 		if (!dimm->bus_prefix) {
> 			rc = -ENOMEM;
> 			goto out;
> 		}
> 		rc =  populate_dimm_attributes(dimm, dimm_base);
>          }

Yes, that looks good, thanks!

> 
> > >  	}
> > >  
> > >  	if (rc == -ENODEV) {
> > > @@ -3506,6 +3526,10 @@ NDCTL_EXPORT int ndctl_cmd_submit(struct ndctl_cmd *cmd)
> > >  		rc = -ENXIO;
> > >  	}
> > >  	close(fd);
> > > +
> > > +	/* update dimm-flags if command submitted successfully */
> > > +	if (!rc && cmd->dimm)
> > > +		ndctl_refresh_dimm_flags(cmd->dimm);
> > >   out:
> > >  	cmd->status = rc;
> > >  	return rc;
> > > diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
> > > index 4d8622978790..e5c56295556d 100644
> > > --- a/ndctl/lib/private.h
> > > +++ b/ndctl/lib/private.h
> > > @@ -75,6 +75,7 @@ struct ndctl_dimm {
> > >  	char *unique_id;
> > >  	char *dimm_path;
> > >  	char *dimm_buf;
> > > +	char *bus_prefix;
> > >  	int health_eventfd;
> > >  	int buf_len;
> > >  	int id;
> > > diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> > > index 4d5cdbf6f619..b1bafd6d9788 100644
> > > --- a/ndctl/libndctl.h
> > > +++ b/ndctl/libndctl.h
> > > @@ -223,6 +223,7 @@ int ndctl_dimm_is_active(struct ndctl_dimm *dimm);
> > >  int ndctl_dimm_is_enabled(struct ndctl_dimm *dimm);
> > >  int ndctl_dimm_disable(struct ndctl_dimm *dimm);
> > >  int ndctl_dimm_enable(struct ndctl_dimm *dimm);
> > > +void ndctl_refresh_dimm_flags(struct ndctl_dimm *dimm);
> > >  
> > >  struct ndctl_cmd;
> > >  struct ndctl_cmd *ndctl_bus_cmd_new_ars_cap(struct ndctl_bus *bus,
> > 
>