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 <[email protected]>
> > > ---
> > > 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.
