Re: [LTP] [PATCH 3/3] cgroup: Add memcontrol02

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

>> +
>> +static void test_memcg_current(unsigned int n)
>> +{
>> +	size_t current;
>> +
>> +	cg_child = tst_cgroup_group_mk(cg_test, "child");
>> +	SAFE_CGROUP_SCANF(cg_child, "memory.current", "%zu", &current);
>> +	TST_EXP_POSITIVE(current == 0, "(current=%zu) == 0", current);
>
> I find these TST_EXP_POSITIVE() macros slightly confusing.
>
> Note that we do have TST_EXP_VAL(), so this can be better defined as
> TST_EXP_VAL(current, 0);
>
> But even then all the macros all written in a way that they do expect
> a syscall as a first parameter and the messages are not clear.

Possibly it should just be TST_EXP(bool_expr, fmt, ...). That would be like
practically every other testing framework. OTOH LTP is somewhat special
as we usually are checking the return value of a syscall. So I should
probably leave these macros alone in this case.

>
> Maybe we need a different solution. We already have tst_assert_foo()
> functions for sysfs/proc files so maybe we need something as compare
> function for the cgroup file attributes:

Frankly that is poor naming. One would expect tst_assert to be similar
to assert from assert.h.

>
> 	enum cmp {
> 		CMP_EQ,
> 		CMP_NE,
> 		CMP_LT,
> 		CMP_GT,
> 		CMP_LE,
> 		CMP_GE,
> 		CMP_EPS,
> 	};
>
> 	CGROUP_ASSERT_CMP_SIZE(cg_child, "memory.current", CMP_EQ, 0);
>
> 	CGROUP_ASSERT_CMP_SIZE(cg_child, "memory.current", CMP_EPS, file, 10);
>
>
> 	or even simple macro that would compare two values accordingly
> 	to the OP and print PASS/FAIL would be better than this.
>

I think it would be simpler to just create a general assert_expr
macro. The above function won't neatly handle loading multiple values
from multiple files. Nor will it handle transforming values.

We could implement SQL queries for sys files, like osquery, that would
be neat!

-- 
Thank you,
Richard.

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