Re: [ndctl PATCH RESEND 2/2] libcxl: Fix accessors for temperature field to support negative value

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

On Mon, 2023-07-31 at 12:17 +0900, Jehoon Park wrote:
> On Mon, Jul 24, 2023 at 09:08:21PM +0000, Verma, Vishal L wrote:
> > On Mon, 2023-07-17 at 15:29 +0900, Jehoon Park wrote:
> > > Add a new macro function to retrieve a signed value such as a temperature.
> > > Replace indistinguishable error numbers with debug message.
> > > 
> > > Signed-off-by: Jehoon Park <jehoon.park@samsung.com>
> > > ---
> > >  cxl/lib/libcxl.c | 36 ++++++++++++++++++++++++++----------
> > >  1 file changed, 26 insertions(+), 10 deletions(-)
> > > 

<..>

> > > 
> > >  CXL_EXPORT int cxl_cmd_health_info_get_temperature(struct cxl_cmd *cmd)
> > >  {
> > >         int rc = health_info_get_temperature_raw(cmd);
> > > +       struct cxl_ctx *ctx = cxl_memdev_get_ctx(cmd->memdev);
> > >  
> > > -       if (rc < 0)
> > > -               return rc;
> > > +       if (rc == 0xffff)
> > > +               dbg(ctx, "%s: Invalid command status\n",
> > > +                   cxl_memdev_get_devname(cmd->memdev));
> > >         if (rc == CXL_CMD_HEALTH_INFO_TEMPERATURE_NOT_IMPL)
> > > -               return -EOPNOTSUPP;
> > > +               dbg(ctx, "%s: Device Temperature not implemented\n",
> > > +                   cxl_memdev_get_devname(cmd->memdev));
> > 
> > Hi Jehoon,
> > 
> > libcxl tends to just return errno codes for simple accessors liek this,
> > and leave it up to the caller to print additional information about why
> > the call might have failed. Even though these are dbg() messages, I'd
> > prefer leaving them out of this patch, and if there is a call site
> > where this fails and there isn't an adequate error message printed as
> > to why, then add these prints there.
> > 
> > Rest of the conversion to s16 looks good.
> > 
> 
> Hi, Vishal.
> 
> Thank you for comment. I agree with the behavior of libcxl accessors as you
> explained. FYI, the reason I replaced errno codes with dbg messages is that
> those accessors are retreiving signed values. I thought returning errno codes
> is not distinguishable from retrieved values when they are negative.
> However, it looks like an overkill because a memory device works below-zero
> temperature would not make sense in real world.
> 
> I'll send revised patch soon after reverting to errno codes and fixing
> related codes in cxl/json.c.
> 
Good point on the negative temperatures - this means we can't use the
negative = error convention, but in this case what you can do is return
something like INT_MAX to indicate an error, and set errno in the
library to whatever error we want to indicate. And adjust all the
callers to check for errors in this way.