Re: [PATCH 03/24] selftests/resctrl: Refactor get_cbm_mask()

[Ilpo Järvinen]

On Thu, 2 Nov 2023, Reinette Chatre wrote:
> On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> 
> > @@ -229,6 +228,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * get_cbm_mask - Get cbm bit mask
> 
> I know you just copied code here but please keep an eye out for acronyms
> to be written in caps.

Yeah, Maciej also commented on this. I've already made some changes but 
I'll incorporate some of your suggestions too.

> This needs not be named to reflect verbatim what the function does.
> Looking ahead I wonder if "get_full_mask()/get_max_mask()" may not be a
> clear indication of what this does?
>
> Something like:
> 	get_full_mask()/get_max_mask()  Get maximum Cache Bit Mask (CBM) 

Having max in the name sounds useful.

Also related to this, the local variables called long_mask should be 
renamed but perhaps not in this series to not block Maciej's work with 
neverending stream of cleanups :-).

> 	@cache_type:	Cache level(? or should this be "type") as "L2" or L3".
> 	@mask:		Full/Maximum portion of cache available for
> 			allocation represented by bit mask
> 			returned as unsigned long.
> 	
> 
> > + * @cache_type:		Cache level L2/L3
> > + * @mask:		cbm_mask returned as unsigned long
> > + *
> > + * Return: = 0 on success, < 0 on failure.
> > + */
> > +int get_cbm_mask(const char *cache_type, unsigned long *mask)
> > +{
> > +	char cbm_mask_path[1024];
> > +	int ret;
> > +
> > +	if (!cache_type)
> > +		return -1;
> 
> Just to confirm ... error checking on mask is intentionally deferred 
> until get_bit_mask()?

I tried to put as much as possible into get_bit_mask() since every caller 
will have to do the same things anyway. I cannot avoid checking cache_type 
here because snprintf() is using it.


Once again, very superb review of the whole series, thank you very much 
for all the effort! It's really appreciated!


-- 
 i.