Re: [ndctl PATCH v2 5/6] ndctl/cxl: Add extent output to region query

[Ira Weiny]

Dave Jiang wrote:

[snip]

> > +static void cxl_extents_init(struct cxl_region *region)
> > +{
> > +   const char *devname = cxl_region_get_devname(region);
> > +   struct cxl_ctx *ctx = cxl_region_get_ctx(region);
> > +   char *extent_path, *dax_region_path;
> > +   struct dirent *de;
> > +   DIR *dir = NULL;
> > +
> > +   if (region->extents_init)
> > +           return;
> > +   region->extents_init = 1;
> > +
> > +   dbg(ctx, "Checking extents: %s\n", region->dev_path);
> > +
> > +   dax_region_path = calloc(1, strlen(region->dev_path) + 64);
> > +   if (!dax_region_path) {
> > +           err(ctx, "%s: allocation failure\n", devname);
> > +           return;
> > +   }
> > +
> > +   extent_path = calloc(1, strlen(region->dev_path) + 100);
> > +   if (!extent_path) {
> > +           err(ctx, "%s: allocation failure\n", devname);
> > +           free(dax_region_path);
> > +           return;
> > +   }
> > +
> > +   sprintf(dax_region_path, "%s/dax_region%d",
> > +           region->dev_path, region->id);
> > +   dir = opendir(dax_region_path);
> > +   if (!dir) {
> > +           err(ctx, "no extents found: %s\n", dax_region_path);
> 
> Also printing the errno may be helpful

Yea. strerror()

> 
> > +           free(extent_path);
> > +           free(dax_region_path);
> > +           return;
> > +   }
> > +
> > +   while ((de = readdir(dir)) != NULL) {
> > +           struct cxl_region_extent *extent;
> > +           char buf[SYSFS_ATTR_SIZE];
> > +           u64 offset, length;
> > +           int id, region_id;
> > +
> > +           if (sscanf(de->d_name, "extent%d.%d", &region_id, &id) != 2)
> > +                   continue;
> > +
> > +           sprintf(extent_path, "%s/extent%d.%d/offset",
> > +                   dax_region_path, region_id, id);
> > +           if (sysfs_read_attr(ctx, extent_path, buf) < 0) {
> > +                   err(ctx, "%s: failed to read extent%d.%d/offset\n",
> > +                           devname, region_id, id);
> > +                   continue;
> > +           }
> > +
> > +           offset = strtoull(buf, NULL, 0);
> > +           if (offset == ERANGE) {
> 
> I think it needs to be coded like:
> if (offset == ULLONG_MAX) {

Yep...  I fell in the habit of the error being returned...

> 
> Not sure why specifically checking for ERANGE, but should be checking against 
> errno for that if needed.

Because I was thinking about strtoull() having odd error handling and was
stupid and did not look at the man page.  If you look closely at the man page
full error coverage needs to be.

        errno = 0;
        offset = strtoull(...);
        if ((offset == ULLONG_MAX) || (offset == 0 && errno == EINVAL)) {
                ...
        }

But sysfs is not going to return an invalid value...  So ULLONG_MAX is all we
need to check for.

Thanks!
Ira

> 
> DJ