On Tue, 2023-02-07 at 19:55 -0800, Ira Weiny wrote:
> Vishal Verma wrote:
<..>
> >
> > diff --git a/cxl/region.c b/cxl/region.c
> > index 38aa142..0945a14 100644
> > --- a/cxl/region.c
> > +++ b/cxl/region.c
> > @@ -380,7 +380,22 @@ static void collect_minsize(struct cxl_ctx *ctx,
> > struct parsed_params *p)
> > struct json_object *jobj =
> > json_object_array_get_idx(p->memdevs, i);
> > struct cxl_memdev *memdev = json_object_get_userdata(jobj);
> > - u64 size = cxl_memdev_get_pmem_size(memdev);
> > + u64 size;
> > +
> > + switch(p->mode) {
> > + case CXL_DECODER_MODE_RAM:
> > + size = cxl_memdev_get_ram_size(memdev);
> > + break;
> > + case CXL_DECODER_MODE_PMEM:
> > + size = cxl_memdev_get_pmem_size(memdev);
> > + break;
> > + default:
> > + /*
> > + * This will 'poison' ep_min_size with a 0, and
> > + * subsequently cause the region creation to fail.
> > + */
> > + size = 0;
>
> Why not change collect_minsize() to return int and propagate the error up
> through create_region_validate_config()?
>
> It seems more confusing to hide a special value in size like this.
>
Hm, true, though the default case should never get hit. In fact I was
planning to leave it out entirely until gcc warned that I can't skip
cases if switching for an enum. I think the comment is maybe misleading
in that it makes one think that this is some special handling. It would
probably be clearer to make size = 0 in the initial declaration, and
make the default case a no-op (maybe with a comment that we would never
get here). Does that sound better?
>
