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

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Thanks Vishal,

I have addressed your review comments in v5 of the patch here at
https://lore.kernel.org/nvdimm/20220222121519.1674117-1-vaibhav@linux.ibm.com

~ Vaibhav

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> 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,
>> > 
>> 
>

-- 
Cheers
~ Vaibhav