Re: [LTP] renameat201.c: Rewrite into new API, split into renameat203.c

[linuxtestproject.agent@gmail.com]

Hi Petr,

thanks for working on the renameat2 conversion/cleanup. The direction looks good, but I don't think this series is ready yet. There are a few must-fix issues here:

- patch 1 adds renameat203.c but does not register it in runtest/syscalls, so the new test never runs;
- patch 1 also drops the old btrfs-specific RENAME_EXCHANGE handling for the ENOENT case, which will regress on the kernels mentioned in the commit message;
- patch 3 moves the fallback into a misleading lapi/stdio.h name and puts a non-static function definition into a public header.

Please see inline comments below.

---
[PATCH 1/3] renameat201.c: Rewrite into new API, split into renameat203.c

BUG: renameat203 is built but never added to runtest/syscalls, so the new test will not be executed by the harness.

> runtest/syscalls
> renameat201 renameat201
> renameat202 renameat202 -i 10

Please add:

> renameat203 renameat203

Without that, the split is incomplete.

BUG: this patch removes the old btrfs-specific handling for RENAME_EXCHANGE, but one of the remaining cases still depends on it.

> +	{NON_EXIST, FLAGS_ERRNO_DESC(RENAME_EXCHANGE, ENOENT)},
> ...
> +	TST_EXP_FAIL(renameat2(olddirfd, TEST_FILE, newdirfd, tc->newpath,
> +			   tc->flags), tc->exp_errno);

On older btrfs kernels, RENAME_EXCHANGE returns EINVAL because the flag is not supported, regardless of whether newpath exists. The old code handled this with a TCONF path:

> if ((test->flags & RENAME_EXCHANGE) && EINVAL == TEST_ERRNO
>     && fs_type == TST_BTRFS_MAGIC)

That exception needs to be restored for the RENAME_EXCHANGE case, otherwise this will fail on the backported kernels mentioned in the commit message.

BUG: fs_type became dead code in renameat201.c.

> +static long fs_type;
> ...
> +	fs_type = tst_fs_type(".");

After removing the btrfs check from renameat2_verify(), fs_type is no longer used. Either restore the compatibility check above or drop fs_type entirely.

BUG: renameat203.c is left with an unfinished TODO in submitted code.

> +static void setup(void)
> +{
> +	// TODO
> +	fs_type = tst_fs_type(".");

Please do not leave a bare TODO in the final patch. Either finish whatever is missing or remove the comment.

minor: desc points to string literals, so this should be const char *.

> +struct tcase {
> +	const char *newpath;
> +	int flags;
> +	int exp_errno;
> +	char *desc;

minor: the macro ordering is a bit confusing here:

> +#define TEST_PATH TEST_DIR TEST_FILE
> +#define TEST_PATH2 TEST_DIR2 TEST_FILE2
> +
> +#define TEST_FILE "test_file"
> +#define TEST_FILE2 "test_file2"

This works because macro expansion is lazy, but it reads backwards. Please define TEST_FILE/TEST_FILE2 before the TEST_PATH helpers.

nit: LTP subjects usually omit the .c suffix, so I would expect:

> renameat201: Rewrite into new API, split into renameat203

Commit message nits:
- "lets keep" -> "let's keep"
- "Due cdd1fedf8261c" -> "Due to cdd1fedf8261c"
- "defacto" -> "de facto"
- "somehow covered" is vague; "also covered" would read better

---
[PATCH 2/3] renameat202.c: Rewrite into new API

nit: typo in the test description.

>  * Verify that :manpage:`renameat2(2)` with RENAME_EXCHANGE swapps the content.

"swapps" -> "swaps"

nit: same subject style comment as patch 1; I'd drop the .c suffix:

> renameat202: Rewrite into new API

Commit message nits:
- "Due cdd1fedf8261c" -> "Due to cdd1fedf8261c"
- "defacto" -> "de facto"
- "Again, as in previous commit," is weak context; I'd restate the actual constraint directly instead of referring back to the previous patch

---
[PATCH 3/3] lapi: Move renameat2.h into lapi/stdio.h

NACK: lapi/stdio.h is the wrong name for this fallback.

> +++ b/include/lapi/stdio.h

stdio.h means standard I/O. renameat2() is a rename syscall, not a stdio interface. Other lapi headers are named after the API they provide (execveat.h, close_range.h, fallocate.h, ...), so this really should be something like lapi/renameat2.h (or, less preferably, lapi/rename.h).

BUG: the fallback definition in a public header must be static inline.

> +#ifndef HAVE_RENAMEAT2
> +int renameat2(int olddirfd, const char *oldpath, int newdirfd,
> +				const char *newpath, unsigned int flags)
> +{
> +	return tst_syscall(__NR_renameat2, olddirfd, oldpath, newdirfd,
> +						newpath, flags);
> +}
> +#endif

All other lapi fallback wrappers use static inline. Without that, including this header in multiple translation units can produce multiple-definition linker failures. For a shared lapi header, that is a real bug and must be fixed before merge.

minor: these includes are not needed for the renameat2() fallback:

> +#include <stdio.h>
> +#include <stdint.h>

The old test-local header only needed config.h, lapi/syscalls.h and sys/types.h, and that is enough here too.

minor: once the header is renamed, the subject should probably be adjusted as well, since the patch does more than just "move renameat2.h"; it also updates the tests to include the new header.

---
So overall this is a NACK for now.

The must-fix items are:
1. add renameat203 to runtest/syscalls;
2. restore the btrfs/RENAME_EXCHANGE compatibility handling in renameat201;
3. rename lapi/stdio.h to something API-appropriate;
4. make the fallback wrapper static inline.

The rest are minor cleanups, but I would also fix them in the respin.

Thanks.

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