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

[Martin Doucha]

On 19. 02. 24 17:09, Petr Vorel wrote:
> 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.

Good catch x2. <sys/stat.h> is needed for umask() but it's included 
through tst_safe_macros.h

> 
>> -#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.)

I planned to have a cleanup() where the fd would be checked and closed 
if needed. But it wasn't needed in the end and I forgot to remove the 
initialization. The variable can be moved inside setup(). Should I send v2?

> 
>> +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).

Zero is still a valid file descriptor whether it's used or not. On the 
other hand, TST_RET will be set by open() in both branches below so the 
initialization is not needed at all.

> 
> 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
>> +};

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic