[LTP] [PATCH v4 2/7] Hugetlb: Migrating libhugetlbfs counters

[Tarun Sahu]

Hi,
Thanks Cyril, for the such a good insights. I will rework on this test.

~Tarun

On Fri, 2022-11-18 at 15:48 +0100, Cyril Hrubis wrote:
> Hi!
> > > > +static long hpage_size;
> > > > +
> > > > +	if (t != et) {
> > > > +		tst_res(TFAIL, "At Line %i:While %s: Bad
> > > > "MEMINFO_HPAGE_TOTAL
> > > > +				" expected %li, actual %li",
> > > > line, desc, et, t);
> > > 
> > > We do have tst_res_() that can be called as:
> > > 
> > > 	tst_res_(__FILE__, line,
> > >                  "%s bad " MEMINFO_HPAGE_TOTAL " = %li expected
> > > %li",
> > > 		 desc, et, t);
> > Ok. Will update it.
> > > > +out:
> > > > +	return verify_counters(line, desc, et, ef, er, es);
> > > > +}
> > > > +#define set_nr_hugepages(c, d) CHECK_(set_nr_hugepages_(c, d,
> > > > __LINE__))
> > > 
> > > The macro name should be uppercase so that it's clear that it's a
> > > macro
> > > and not a simple function. With that we can also drop the
> > > underscore
> > > from the actual function name too.
> > > 
> > I avoided it deliberately knowing that these macros are being
> > called too
> > often, keeping them in uppercase would have made the code look too
> > messy.
> > Then, I looked that there was tst_res and similiar macros too which
> > were
> > kept in smallcase too (which is wrapper to tst_res_ function), That
> > is why
> > I kept them smallcase.
> 
> The whole point why we are going in circles around how to structure
> this
> piece of code is code readibility. Source code is more often read
> than
> written, which means that it's more important to invest into writing
> code that is easily understandable since that will save effort in the
> long term. That is true especially for kernel tests, where there is
> enough complexity in the kernel itself and writing tests that are not
> easy to understand does actually harm.
> 
> There is a key difference between tst_res() defined to tst_res_()
> that
> adds a few parameters and macros that change the codeflow. If there
> are
> macros that change code flow, and perhaps checkpatch is right here
> that
> it's wiser to avoid them at all, they should at least scream in upper
> case letters that this is not a regular function.
> 
> This all boils down to a principle of a least surprise.
> 
> Perhaps the best solution would be to get back to a drawing board and
> figure out how to better structure the test so that the code flow is
> easier to follow.
> 


> > > Maybe it would be a bit nicer to have actually two different
> > > macros so
> > > that we don't have to resort to this do {} while (0) trickery
> > > e.g.
> > > 
> > > #define CHECK_BREAK(cond) if (cond) break;
> > > #define CHECK_RETURN(cond) if (cond) return -1;
> > > 
> > > #define MAP_BREAK(s, h, f, d) CHECK_BREAK(map(s, h, f, d,
> > > __LINE__))
> > > #define MAP_RETURN(s, h, f, d) CHECK_RETURN(map(s, h, f, d,
> > > __LINE__))
> > > 
> > > Then the check counters would look much better.
> > > 
> > > Or we can just stick to the RETURN variants and put the body of
> > > the loop
> > > in the runtest() function into do_interation() function.
> > > 
> > I like the second idea, as it will only have one macro. which will
> > keep the
> > code less messy. But anyway, I had tried this already, if (cond)
> > return; is
> > not allowed. "make check" throws warning saying, "Macros with flow
> > control
> > statements should be avoided". Only way I could see is to use
> > break.
> 
> Again, let's not work around the tooling we have, the tooling is
> supposed to help, once you start bending the code so that the tooling
> is
> happy you are on a wrong path.
>