Verma, Vishal L wrote:
> 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?
> >
To me the question is whether size == 0 is an error or just a valid size
which something at the higher level determines is an error. To me the
default case here is that mode is incorrectly set to get a valid size.
Your comment implied that as well.
Having size == 0 is ok generally as this is a 'min size'. So I will not
argue the point too much.
Ira
