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

[Cyril Hrubis <chrubis@suse.cz>]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp