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

[Petr Vorel <pvorel@suse.cz>]

Hi Martin,

TL;DR: merged with fixes.

> 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

Ah, good point. I removed it before merge (I hope we never reorganize lib/*.c
that much that we'd remove some includes in include/*.h, because quite a few
tests depends on these indirect includes already).

> > > -#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?

Nah, fixed before merge. +1 for pointing out it was for setup() only.

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

+1

Removed and merged.

Thanks for taking care for these tiny details.

Kind regards,
Petr

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