Re: [LTP] [PATCH] Migrating the libhugetlbfs/testcases/quota.c

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Pavithra,

Consider this as a simple first-looking review. Probably more will come
when you will send more patches.

Thanks for working on migrating this test. A few issues below that
need to be addressed before this can be merged.

> +/*\
> + * [Description]
> + *

The [Description] tag is deprecated and must be removed. Just start
the doc comment directly with the text.

> +#define _GNU_SOURCE
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <sys/vfs.h>
> +#include <sys/statfs.h>
> +#include <sys/mount.h>
> +
> +#include "hugetlb.h"
> +#include "tst_safe_macros.h"

The "tst_safe_macros.h" include is redundant since tst_test.h (pulled
in by hugetlb.h) already includes it. Please drop it.

> +static void verify_quota_stat(int line, long tot, long free, long avail)
> +{
> +[...]
> +		tst_res_(NULL, line, TFAIL,

> +#define VERIFY_QUOTA_STAT(t, f, a) verify_quota_stat(__LINE__, t, f, a)

> static void do_map(unsigned long size, int mmap_flags, int action_flags)

In this function it's better to use goto to cleanup memory at the end of
it. So we reduce nesting after mmap() failure. Also, should we TBROK mmap()
when there's no quota pressure but the syscall fails? At the moment we are
failing for mmap() for any type of errno.

> +static void test_quota_scenario(int line, int expected_result,
> +[...]
> +		tst_res_(NULL, line, TFAIL,

> +#define TEST_QUOTA(exp, size, flags, actions) \
> +	test_quota_scenario(__LINE__, exp, size, flags, actions)

tst_res_() is an internal LTP API and must not be used directly by
tests. Drop the __LINE__ parameter and the wrapper macros, and use
plain tst_res(TFAIL, ...) instead. The failure messages already
contain enough context (quota counter values, expected vs actual
result) to identify which check failed without needing line numbers.

> +	if (actual_result != expected_result) {
> +		const char *result_names[] = {"success", "signal", "failure"};
> +		tst_res_(NULL, line, TFAIL,

checkpatch flags two issues here:
  - result_names[] should be static const
  - missing blank line after the declaration

> +	if (mount("none", quota_mnt, "hugetlbfs", 0, mount_opts) == -1) {
> +		if (errno == ENOSYS || errno == ENODEV)
> +			tst_brk(TCONF, "hugetlbfs not supported");

checkpatch warns about ENOSYS here. ENOSYS means "invalid syscall
number" and mount() will not return it. ENODEV alone is sufficient
to detect missing hugetlbfs support.

> +	.needs_tmpdir = 1,

The test already has .mntpoint and .needs_hugetlbfs set, so it gets a
temporary directory via the hugetlbfs mount infrastructure. Is
.needs_tmpdir = 1 actually needed here? If the test does not create
files outside MNTPOINT (it does not appear to), this can be dropped.


Also, in general, I think that we should have multiple test cases for
different types of quota, instead of calling the `test_quota_scenario()`
with different parameters one after another. In this way we can organise
test in a better way and reduce redundancy inside the code.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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