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