Re: [LTP] [PATCH] open07: Convert to new API

[Petr Vorel <pvorel@suse.cz>]

Hi Martin,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few minor things below, can be fixed before merge.

...
>  #define _GNU_SOURCE		/* for O_NOFOLLOW */
nit: This now works without _GNU_SOURCE (we compile with -std=gnu99 and it would
work for whatever gnu*).

> -#include <stdio.h>
> -#include <errno.h>
> -#include <sys/types.h>
>  #include <sys/stat.h>
nit: IMHO <sys/stat.h> was not needed even in the old API version.

> -#include <fcntl.h>
> -#include "test.h"
> -#include "safe_macros.h"
...
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +
> +#define TESTFILE "testfile"
> +#define TESTDIR "testdir"
> +#define SYMFILE1 "symfile1"
> +#define SYMFILE2 "symfile2"
> +#define SYMDIR1 "symdir1"
> +#define SYMDIR2 "symdir2"
> +#define PASSFILE "symdir1/testfile"
> +
> +static int fd = -1;
nit: any reason for -1? (We don't check the input.)

> +static struct testcase {
> +	const char *path;
> +	int err;
> +	const char *desc;
> +} testcase_list[] = {
> +	{SYMFILE1, ELOOP, "open(O_NOFOLLOW) a symlink to file"},
> +	{SYMFILE2, ELOOP, "open(O_NOFOLLOW) a double symlink to file"},
> +	{SYMDIR1, ELOOP, "open(O_NOFOLLOW) a symlink to directory"},
> +	{SYMDIR2, ELOOP, "open(O_NOFOLLOW) a double symlink to directory"},
> +	{PASSFILE, 0, "open(O_NOFOLLOW) a file in symlinked directory"},

...
> +static void setup(void)
>  {
> -	char file1[100], file2[100];
> -
> -	sprintf(file1, "open11.3.%d", getpid());
> -	sprintf(file2, "open12.4.%d", getpid());
> -	SAFE_MKDIR(cleanup, file1, 00700);
> +	umask(0);
> +	fd = SAFE_CREAT(TESTFILE, 0644);
> +	SAFE_CLOSE(fd);
> +	SAFE_MKDIR(TESTDIR, 0755);

> -	SAFE_SYMLINK(cleanup, file1, file2);
> +	SAFE_SYMLINK(TESTFILE, SYMFILE1);
> +	SAFE_SYMLINK(SYMFILE1, SYMFILE2);
> +	SAFE_SYMLINK(TESTDIR, SYMDIR1);
> +	SAFE_SYMLINK(SYMDIR1, SYMDIR2);

> -	strcpy(TC[4].filename, file2);
> -	strcat(TC[4].filename, "/");
> +	fd = SAFE_CREAT(PASSFILE, 0644);
> +	SAFE_CLOSE(fd);
>  }

> -static void setup(void)
> +static void run(unsigned int n)
>  {
> -	umask(0);
> +	const struct testcase *tc = testcase_list + n;

> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> +	TST_RET = -1;
nit: IMHO this is not needed (we have 0 for stdin, right? Therefore open()
should not get 0 and check below is correct).

Kind regards,
Petr

> -	TEST_PAUSE;
> +	if (tc->err) {
> +		TST_EXP_FAIL2(open(tc->path, O_NOFOLLOW | O_RDONLY), tc->err,
> +			"%s", tc->desc);
> +	} else {
> +		TST_EXP_FD(open(tc->path, O_NOFOLLOW | O_RDONLY),
> +			"%s", tc->desc);
> +	}

> -	tst_tmpdir();
> +	if (TST_RET >= 0)
> +		SAFE_CLOSE(TST_RET);
>  }

...
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test = run,
> +	.tcnt = ARRAY_SIZE(testcase_list),
> +	.needs_tmpdir = 1
> +};

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