[LTP] [PATCH] fcntl37: test posix lock across execve

[LTP] [PATCH] fcntl37: test posix lock across execve

[Xiong Zhou]

Signed-off-by: Xiong Zhou <xzhou@redhat.com>
---
 testcases/kernel/syscalls/fcntl/Makefile  |   3 +
 testcases/kernel/syscalls/fcntl/fcntl37.c | 146 ++++++++++++++++++++++++++++++
 2 files changed, 149 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c

diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
index ae37214..7a06aa7 100644
--- a/testcases/kernel/syscalls/fcntl/Makefile
+++ b/testcases/kernel/syscalls/fcntl/Makefile
@@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread
 fcntl36: LDLIBS += -lpthread
 fcntl36_64: LDLIBS += -lpthread
 
+fcntl37: LDLIBS += -lpthread
+fcntl37_64: LDLIBS += -lpthread
+
 include $(top_srcdir)/include/mk/testcases.mk
 include $(abs_srcdir)/../utils/newer_64.mk
 
diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c
new file mode 100644
index 0000000..bac2168
--- /dev/null
+++ b/testcases/kernel/syscalls/fcntl/fcntl37.c
@@ -0,0 +1,146 @@
+/*
+ * Copyright (c) 2018 RedHat Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Xiong Zhou <xzhou@redhat.com>
+ *
+ * This is testing for
+ *
+ *	"Record locks are not inherited by a child created via fork(2),
+ *	 but are preserved across an execve(2)."
+ *
+ * from fcntl(2) man page.
+ *
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <sched.h>
+#include <limits.h>
+
+#include "lapi/fcntl.h"
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
+
+static const char fname[] = "tst_lock_execve";
+static const char flag_fname[] = "/tmp/execved";
+static void cleanup(void);
+
+static void *thread_fn(void *arg)
+{
+	int fd = *(int *)arg;
+	tst_res(TINFO, "Thread running. fd %d", fd);
+	/* Just need to be alive when execve. */
+	sleep(5);
+	SAFE_CLOSE(fd);
+	return NULL;
+}
+
+static void checklock(int fd)
+{
+	pid_t pid = SAFE_FORK();
+	if (pid == 0) {
+		struct flock flck = {
+			.l_type = F_WRLCK,
+			.l_whence = SEEK_SET,
+			.l_start = 0,
+			.l_len = 1,
+		};
+		SAFE_FCNTL(fd, F_GETLK, &flck);
+		if (flck.l_type == F_UNLCK)
+			tst_res(TFAIL, "Record lock gets lost after execve");
+		else
+			tst_res(TPASS, "Record lock survives execve");
+		SAFE_CLOSE(fd);
+		_exit(0);
+	}
+	waitpid(pid, NULL, 0);
+}
+
+static void test01(void)
+{
+	int fd, ret;
+	struct stat stat_buf;
+
+	/*
+	 * If tmp/tst_lock_execve exists, execve to run checklock.
+	 */
+	ret = stat(flag_fname, &stat_buf);
+	if (ret == 0) {
+		checklock(fd);
+		cleanup();
+		_exit(0);
+	}
+
+	/*
+	 * If tmp/tst_lock_execve does not exist, initialize it.
+	 */
+	SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755);
+	fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755);
+	struct flock64 flck = {
+		.l_type = F_WRLCK,
+		.l_whence = SEEK_SET,
+		.l_start  = 0,
+		.l_len    = 1,
+	};
+	SAFE_FCNTL(fd, F_SETLK, &flck);
+
+	/*
+	 * Creat thread and keep it running after placing posix
+	 * write lock.
+	 */
+	pthread_t th;
+	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
+	sleep(1);
+
+	/*
+	 * Clear CLOEXEC
+	 */
+	int flags=SAFE_FCNTL(fd, F_GETFD);
+	flags &= ~FD_CLOEXEC;
+	SAFE_FCNTL(fd, F_SETFD, flags);
+
+	/*
+	 * Get full path name of running programm then execve.
+	 */
+	char prog_name[PATH_MAX];
+	ret = readlink("/proc/self/exe", prog_name, PATH_MAX);
+	char * const newargv[] = { prog_name, NULL, NULL };
+	tst_res(TINFO, "execve %s with %s locked", prog_name, fname);
+	execve(prog_name, newargv, NULL);
+	/*
+	 * Failure info for debug
+	 */
+	perror("execve ");
+}
+
+static void cleanup(void)
+{
+	SAFE_UNLINK(flag_fname);
+}
+
+static struct tst_test test = {
+	.needs_tmpdir = 1,
+	.forks_child = 1,
+	.test_all = test01,
+	.cleanup = cleanup,
+};
-- 
1.8.3.1

[LTP] [PATCH] fcntl37: test posix lock across execve

[Daniel P. =?unknown-8bit?q?Berrang=C3=A9?=]

On Mon, Mar 26, 2018 at 09:28:15PM +0800, Xiong Zhou wrote:
> Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> ---
>  testcases/kernel/syscalls/fcntl/Makefile  |   3 +
>  testcases/kernel/syscalls/fcntl/fcntl37.c | 146 ++++++++++++++++++++++++++++++
>  2 files changed, 149 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c
> 
> diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
> index ae37214..7a06aa7 100644
> --- a/testcases/kernel/syscalls/fcntl/Makefile
> +++ b/testcases/kernel/syscalls/fcntl/Makefile
> @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread
>  fcntl36: LDLIBS += -lpthread
>  fcntl36_64: LDLIBS += -lpthread
>  
> +fcntl37: LDLIBS += -lpthread
> +fcntl37_64: LDLIBS += -lpthread
> +
>  include $(top_srcdir)/include/mk/testcases.mk
>  include $(abs_srcdir)/../utils/newer_64.mk
>  
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c
> new file mode 100644
> index 0000000..bac2168
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fcntl/fcntl37.c
> @@ -0,0 +1,146 @@
> +/*
> + * Copyright (c) 2018 RedHat Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Author: Xiong Zhou <xzhou@redhat.com>
> + *
> + * This is testing for
> + *
> + *	"Record locks are not inherited by a child created via fork(2),
> + *	 but are preserved across an execve(2)."
> + *
> + * from fcntl(2) man page.
> + *
> + */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <pthread.h>
> +#include <sched.h>
> +#include <limits.h>
> +
> +#include "lapi/fcntl.h"
> +#include "tst_safe_pthread.h"
> +#include "tst_test.h"
> +
> +static const char fname[] = "tst_lock_execve";
> +static const char flag_fname[] = "/tmp/execved";
>
> +static void cleanup(void);
> +
> +static void *thread_fn(void *arg)
> +{
> +	int fd = *(int *)arg;
> +	tst_res(TINFO, "Thread running. fd %d", fd);
> +	/* Just need to be alive when execve. */
> +	sleep(5);

Since we don't expect this thread to complete running before execve() takes
place, feels more robust to put put it into an infinite loop eg

  while (1) sleep (1);

> +	SAFE_CLOSE(fd);

and ditch this since it is not supposed to be reachable if the test is
operating correctly.

> +	return NULL;
> +}
> +
> +static void checklock(int fd)
> +{
> +	pid_t pid = SAFE_FORK();
> +	if (pid == 0) {
> +		struct flock flck = {
> +			.l_type = F_WRLCK,
> +			.l_whence = SEEK_SET,
> +			.l_start = 0,
> +			.l_len = 1,
> +		};
> +		SAFE_FCNTL(fd, F_GETLK, &flck);
> +		if (flck.l_type == F_UNLCK)
> +			tst_res(TFAIL, "Record lock gets lost after execve");
> +		else
> +			tst_res(TPASS, "Record lock survives execve");
> +		SAFE_CLOSE(fd);
> +		_exit(0);
> +	}
> +	waitpid(pid, NULL, 0);
> +}
> +
> +static void test01(void)
> +{
> +	int fd, ret;
> +	struct stat stat_buf;
> +
> +	/*
> +	 * If tmp/tst_lock_execve exists, execve to run checklock.
> +	 */
> +	ret = stat(flag_fname, &stat_buf);
> +	if (ret == 0) {
> +		checklock(fd);

Nothing seems to have initialized the 'fd' variable when it is
used here.

> +		cleanup();
> +		_exit(0);
> +	}
> +
> +	/*
> +	 * If tmp/tst_lock_execve does not exist, initialize it.
> +	 */
> +	SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755);
> +	fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755);

What's the benefit in using 2 temporary files - feels like using
one would be sufficient, particularly since this code is never
calling unlink(fname) so leaking the 2nd file on disk

> +	struct flock64 flck = {
> +		.l_type = F_WRLCK,
> +		.l_whence = SEEK_SET,
> +		.l_start  = 0,
> +		.l_len    = 1,
> +	};
> +	SAFE_FCNTL(fd, F_SETLK, &flck);
> +
> +	/*
> +	 * Creat thread and keep it running after placing posix
> +	 * write lock.
> +	 */
> +	pthread_t th;
> +	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
> +	sleep(1);
> +
> +	/*
> +	 * Clear CLOEXEC
> +	 */
> +	int flags=SAFE_FCNTL(fd, F_GETFD);
> +	flags &= ~FD_CLOEXEC;
> +	SAFE_FCNTL(fd, F_SETFD, flags);
> +
> +	/*
> +	 * Get full path name of running programm then execve.
> +	 */
> +	char prog_name[PATH_MAX];
> +	ret = readlink("/proc/self/exe", prog_name, PATH_MAX);
> +	char * const newargv[] = { prog_name, NULL, NULL };
> +	tst_res(TINFO, "execve %s with %s locked", prog_name, fname);
> +	execve(prog_name, newargv, NULL);
> +	/*
> +	 * Failure info for debug
> +	 */
> +	perror("execve ");
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_UNLINK(flag_fname);
> +}
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +	.test_all = test01,
> +	.cleanup = cleanup,
> +};
> -- 
> 1.8.3.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[LTP] [PATCH] fcntl37: test posix lock across execve

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
> index ae37214..7a06aa7 100644
> --- a/testcases/kernel/syscalls/fcntl/Makefile
> +++ b/testcases/kernel/syscalls/fcntl/Makefile
> @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread
>  fcntl36: LDLIBS += -lpthread
>  fcntl36_64: LDLIBS += -lpthread
>  
> +fcntl37: LDLIBS += -lpthread
> +fcntl37_64: LDLIBS += -lpthread

This is wrong, we should use CFLAGS += -pthread instead, also the rest
of the Makefile should be fixed as well.

>  include $(top_srcdir)/include/mk/testcases.mk
>  include $(abs_srcdir)/../utils/newer_64.mk

...

> +#include "lapi/fcntl.h"
> +#include "tst_safe_pthread.h"
> +#include "tst_test.h"
> +
> +static const char fname[] = "tst_lock_execve";
> +static const char flag_fname[] = "/tmp/execved";
                                       ^
				     You must not create files in random
				     places. Once you set the
				     .needs_tmpdir in the test structure
				     the test is executed with unique temporary
				     directory as working directory so
				     all you need to do for temporary
				     files is to work with relative filenames.

> +static void cleanup(void);
> +
> +static void *thread_fn(void *arg)
> +{
> +	int fd = *(int *)arg;
                   ^
		   This is broken, you have to use intptr_t

> +	tst_res(TINFO, "Thread running. fd %d", fd);
> +	/* Just need to be alive when execve. */
> +	sleep(5);
> +	SAFE_CLOSE(fd);
> +	return NULL;
> +}
> +
> +static void checklock(int fd)
> +{
> +	pid_t pid = SAFE_FORK();
> +	if (pid == 0) {
> +		struct flock flck = {
> +			.l_type = F_WRLCK,
> +			.l_whence = SEEK_SET,
> +			.l_start = 0,
> +			.l_len = 1,
> +		};
> +		SAFE_FCNTL(fd, F_GETLK, &flck);
> +		if (flck.l_type == F_UNLCK)
> +			tst_res(TFAIL, "Record lock gets lost after execve");
> +		else
> +			tst_res(TPASS, "Record lock survives execve");
> +		SAFE_CLOSE(fd);
> +		_exit(0);

Why _exit() ?

> +	}
> +	waitpid(pid, NULL, 0);

SAFE_WAITPID()

> +}

Also having the check in a separate function and calling it several
times makes it kind of hard for debugging. if one of the assertions
fails the error messages would be the same. Maybe we should pass a
string with describes assertion type to the function and use it
int the tst_res() messages.

> +static void test01(void)
> +{
> +	int fd, ret;
> +	struct stat stat_buf;
> +
> +	/*
> +	 * If tmp/tst_lock_execve exists, execve to run checklock.
> +	 */
> +	ret = stat(flag_fname, &stat_buf);
> +	if (ret == 0) {
> +		checklock(fd);
> +		cleanup();

You must not call the cleanup from the test. Cleanup function is called
from the test library.

> +		_exit(0);

Again why _exit() ?

> +	}

Don't do this.

You must not reexecute the testcase and put file flags into random
places. If you need execve() a test helper, you have to write simple
binary helper. See testcases/kernel/syscalls/creat/creat07_child.c

> +	/*
> +	 * If tmp/tst_lock_execve does not exist, initialize it.
> +	 */

Please no useless comments like this one.

> +	SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755);
> +	fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755);
> +	struct flock64 flck = {
> +		.l_type = F_WRLCK,
> +		.l_whence = SEEK_SET,
> +		.l_start  = 0,
> +		.l_len    = 1,
> +	};
> +	SAFE_FCNTL(fd, F_SETLK, &flck);
> +
> +	/*
> +	 * Creat thread and keep it running after placing posix
> +	 * write lock.
> +	 */

Please no useless comments.

> +	pthread_t th;
> +	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
                                                    ^
						    Here as well, we
						    have to cast the fd
						    to intptr_t first.
> +	sleep(1);

Please no random sleeps() for thread synchronization, either use real
thread synchronization primitives or LTP checkpoint synchronization
library which is based on futexes.

See:
https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#229-fork-and-parent-child-synchronization

> +	/*
> +	 * Clear CLOEXEC
> +	 */

Please no useless comments.

> +	int flags=SAFE_FCNTL(fd, F_GETFD);
> +	flags &= ~FD_CLOEXEC;
> +	SAFE_FCNTL(fd, F_SETFD, flags);
> +
> +	/*
> +	 * Get full path name of running programm then execve.
> +	 */

Please no useless comments.


> +	char prog_name[PATH_MAX];
> +	ret = readlink("/proc/self/exe", prog_name, PATH_MAX);
> +	char * const newargv[] = { prog_name, NULL, NULL };

This is far to complicated for no reason. The path to LTP test binaries
is in $PATH so you can execute any LTP binary just by it's name.
Absolute path is absolutely unneeded.

> +	tst_res(TINFO, "execve %s with %s locked", prog_name, fname);
> +	execve(prog_name, newargv, NULL);
> +	/*
> +	 * Failure info for debug
> +	 */

Please no useless comments.

> +	perror("execve ");

No perror() please. Any error messages should be printed via tst_res()
or tst_brk().

> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_UNLINK(flag_fname);
> +}
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.forks_child = 1,
> +	.test_all = test01,
> +	.cleanup = cleanup,
> +};
> -- 
> 1.8.3.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl37: test posix lock across execve

[Cyril Hrubis]

Hi!
> > +	pthread_t th;
> > +	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
>                                                     ^
> 						    Here as well, we
> 						    have to cast the fd
> 						    to intptr_t first.

Scratch that, you are passing the fd by pointer not by value, which is
questionable practice for a value on the function stack.

What is usually done when you need to pass integers to threads as
paramters is to cast the integer to a type with the same width as a
pointer then casting it to a void*, which is (void*)(intptr_t)fd in this
case.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl37: test posix lock across execve

[Xiong Zhou]

Hi,

Thanks for reviewing!

On Mon, Mar 26, 2018 at 04:33:33PM +0200, Cyril Hrubis wrote:
> Hi!
> > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
> > index ae37214..7a06aa7 100644
> > --- a/testcases/kernel/syscalls/fcntl/Makefile
> > +++ b/testcases/kernel/syscalls/fcntl/Makefile
> > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread
> >  fcntl36: LDLIBS += -lpthread
> >  fcntl36_64: LDLIBS += -lpthread
> >  
> > +fcntl37: LDLIBS += -lpthread
> > +fcntl37_64: LDLIBS += -lpthread
> 
> This is wrong, we should use CFLAGS += -pthread instead, also the rest
				^ This is not working.

gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64  -c -o fcntl37_64.o fcntl37.c
gcc   -L../../../../lib  fcntl37_64.o   -lltp -o fcntl37_64
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create':
/home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create'
../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join':
/home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join'
collect2: error: ld returned 1 exit status


> of the Makefile should be fixed as well.
> 
> >  include $(top_srcdir)/include/mk/testcases.mk
> >  include $(abs_srcdir)/../utils/newer_64.mk
> 
> ...
> 
> > +#include "lapi/fcntl.h"
> > +#include "tst_safe_pthread.h"
> > +#include "tst_test.h"
> > +
> > +static const char fname[] = "tst_lock_execve";
> > +static const char flag_fname[] = "/tmp/execved";
>                                        ^
> 				     You must not create files in random
> 				     places. Once you set the
> 				     .needs_tmpdir in the test structure
> 				     the test is executed with unique temporary
> 				     directory as working directory so
> 				     all you need to do for temporary
> 				     files is to work with relative filenames.
> 
> > +static void cleanup(void);
> > +
> > +static void *thread_fn(void *arg)
> > +{
> > +	int fd = *(int *)arg;
>                    ^
> 		   This is broken, you have to use intptr_t
> 
> > +	tst_res(TINFO, "Thread running. fd %d", fd);
> > +	/* Just need to be alive when execve. */
> > +	sleep(5);
> > +	SAFE_CLOSE(fd);
> > +	return NULL;
> > +}
> > +
> > +static void checklock(int fd)
> > +{
> > +	pid_t pid = SAFE_FORK();
> > +	if (pid == 0) {
> > +		struct flock flck = {
> > +			.l_type = F_WRLCK,
> > +			.l_whence = SEEK_SET,
> > +			.l_start = 0,
> > +			.l_len = 1,
> > +		};
> > +		SAFE_FCNTL(fd, F_GETLK, &flck);
> > +		if (flck.l_type == F_UNLCK)
> > +			tst_res(TFAIL, "Record lock gets lost after execve");
> > +		else
> > +			tst_res(TPASS, "Record lock survives execve");
> > +		SAFE_CLOSE(fd);
> > +		_exit(0);
> 
> Why _exit() ?
> 
> > +	}
> > +	waitpid(pid, NULL, 0);
> 
> SAFE_WAITPID()
> 
> > +}
> 
> Also having the check in a separate function and calling it several
> times makes it kind of hard for debugging. if one of the assertions
> fails the error messages would be the same. Maybe we should pass a
> string with describes assertion type to the function and use it
> int the tst_res() messages.
> 
> > +static void test01(void)
> > +{
> > +	int fd, ret;
> > +	struct stat stat_buf;
> > +
> > +	/*
> > +	 * If tmp/tst_lock_execve exists, execve to run checklock.
> > +	 */
> > +	ret = stat(flag_fname, &stat_buf);
> > +	if (ret == 0) {
> > +		checklock(fd);
> > +		cleanup();
> 
> You must not call the cleanup from the test. Cleanup function is called
> from the test library.
> 
> > +		_exit(0);
> 
> Again why _exit() ?
> 
> > +	}
> 
> Don't do this.
> 
> You must not reexecute the testcase and put file flags into random
> places. If you need execve() a test helper, you have to write simple
> binary helper. See testcases/kernel/syscalls/creat/creat07_child.c

Thanks for the pointer. If I can define TST_NO_DEFAULT_MAIN and have
main() function in this testcase, file flag is not needed.
Passing args would do the trick, as Daniel's original reproducer.

Thanks,
Xiong
> 
> > +	/*
> > +	 * If tmp/tst_lock_execve does not exist, initialize it.
> > +	 */
> 
> Please no useless comments like this one.
> 
> > +	SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755);
> > +	fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755);
> > +	struct flock64 flck = {
> > +		.l_type = F_WRLCK,
> > +		.l_whence = SEEK_SET,
> > +		.l_start  = 0,
> > +		.l_len    = 1,
> > +	};
> > +	SAFE_FCNTL(fd, F_SETLK, &flck);
> > +
> > +	/*
> > +	 * Creat thread and keep it running after placing posix
> > +	 * write lock.
> > +	 */
> 
> Please no useless comments.
> 
> > +	pthread_t th;
> > +	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
>                                                     ^
> 						    Here as well, we
> 						    have to cast the fd
> 						    to intptr_t first.
> > +	sleep(1);
> 
> Please no random sleeps() for thread synchronization, either use real
> thread synchronization primitives or LTP checkpoint synchronization
> library which is based on futexes.
> 
> See:
> https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#229-fork-and-parent-child-synchronization
> 
> > +	/*
> > +	 * Clear CLOEXEC
> > +	 */
> 
> Please no useless comments.
> 
> > +	int flags=SAFE_FCNTL(fd, F_GETFD);
> > +	flags &= ~FD_CLOEXEC;
> > +	SAFE_FCNTL(fd, F_SETFD, flags);
> > +
> > +	/*
> > +	 * Get full path name of running programm then execve.
> > +	 */
> 
> Please no useless comments.
> 
> 
> > +	char prog_name[PATH_MAX];
> > +	ret = readlink("/proc/self/exe", prog_name, PATH_MAX);
> > +	char * const newargv[] = { prog_name, NULL, NULL };
> 
> This is far to complicated for no reason. The path to LTP test binaries
> is in $PATH so you can execute any LTP binary just by it's name.
> Absolute path is absolutely unneeded.
> 
> > +	tst_res(TINFO, "execve %s with %s locked", prog_name, fname);
> > +	execve(prog_name, newargv, NULL);
> > +	/*
> > +	 * Failure info for debug
> > +	 */
> 
> Please no useless comments.
> 
> > +	perror("execve ");
> 
> No perror() please. Any error messages should be printed via tst_res()
> or tst_brk().
> 
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +	SAFE_UNLINK(flag_fname);
> > +}
> > +
> > +static struct tst_test test = {
> > +	.needs_tmpdir = 1,
> > +	.forks_child = 1,
> > +	.test_all = test01,
> > +	.cleanup = cleanup,
> > +};
> > -- 
> > 1.8.3.1
> > 
> > 
> > -- 
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH] fcntl37: test posix lock across execve

[Cyril Hrubis]

Hi!
> > > +fcntl37: LDLIBS += -lpthread
> > > +fcntl37_64: LDLIBS += -lpthread
> > 
> > This is wrong, we should use CFLAGS += -pthread instead, also the rest
> 				^ This is not working.
> 
> gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64  -c -o fcntl37_64.o fcntl37.c
> gcc   -L../../../../lib  fcntl37_64.o   -lltp -o fcntl37_64
> ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create':
> /home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create'
> ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join':
> /home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join'
> collect2: error: ld returned 1 exit status

Hmm, looks like something is wrong with the newer_64.mk. It picks up a
linker rule that does not include CFLAGS. Following patch fixes that,
but I'm not 100% sure that it's correct, I will look further into that.

diff --git a/testcases/kernel/syscalls/utils/newer_64.mk b/testcases/kernel/syscalls/utils/newer_64.mk
index 8cd7e03c8..c70c2af53 100644
--- a/testcases/kernel/syscalls/utils/newer_64.mk
+++ b/testcases/kernel/syscalls/utils/newer_64.mk
@@ -49,7 +49,9 @@ HAS_NEWER_64          := 0
 endif
 
 %_64: CFLAGS += -D$(DEF_64)=1
-# XXX (garrcoop): End section of code in question..
 
 %_64.o: %.c
        $(COMPILE.c) $(OUTPUT_OPTION) $<
+
+%_64: %_64.o
+       $(LINK.c) $(OUTPUT_OPTION) $< $(LDLIBS)


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl37: test posix lock across execve

[Xiong Zhou]

On Tue, Mar 27, 2018 at 12:19:29PM +0200, Cyril Hrubis wrote:
> Hi!
> > > > +fcntl37: LDLIBS += -lpthread
> > > > +fcntl37_64: LDLIBS += -lpthread
> > > 
> > > This is wrong, we should use CFLAGS += -pthread instead, also the rest
> > 				^ This is not working.
> > 
> > gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64  -c -o fcntl37_64.o fcntl37.c
> > gcc   -L../../../../lib  fcntl37_64.o   -lltp -o fcntl37_64
> > ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create':
> > /home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create'
> > ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join':
> > /home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join'
> > collect2: error: ld returned 1 exit status
> 
> Hmm, looks like something is wrong with the newer_64.mk. It picks up a
> linker rule that does not include CFLAGS. Following patch fixes that,
> but I'm not 100% sure that it's correct, I will look further into that.
> 
> diff --git a/testcases/kernel/syscalls/utils/newer_64.mk b/testcases/kernel/syscalls/utils/newer_64.mk
> index 8cd7e03c8..c70c2af53 100644
> --- a/testcases/kernel/syscalls/utils/newer_64.mk
> +++ b/testcases/kernel/syscalls/utils/newer_64.mk
> @@ -49,7 +49,9 @@ HAS_NEWER_64          := 0
>  endif
>  
>  %_64: CFLAGS += -D$(DEF_64)=1
> -# XXX (garrcoop): End section of code in question..
>  
>  %_64.o: %.c
>         $(COMPILE.c) $(OUTPUT_OPTION) $<
> +
> +%_64: %_64.o
> +       $(LINK.c) $(OUTPUT_OPTION) $< $(LDLIBS)

This patch works fine.

Thanks,
Xiong

> 
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH v2] fcntl37: test posix lock across execve

[Xiong Zhou]

Signed-off-by: Xiong Zhou <xzhou@redhat.com>
---

v2:
	s/LDLIBS/CFLAGS/ in Makefile.
	No fork() and no another function when checking the lock.
	TST_NO_DEFAULT_MAIN to pass fd across execve.
	Update author info. It's from Daniel originally. I copied
from fcntl36.c for v1 patch and forgot to update it. Sorry for
missing this.

 testcases/kernel/syscalls/fcntl/Makefile  |  11 ++--
 testcases/kernel/syscalls/fcntl/fcntl37.c | 101 ++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 4 deletions(-)
 create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c

diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
index ae37214..7a215c8 100644
--- a/testcases/kernel/syscalls/fcntl/Makefile
+++ b/testcases/kernel/syscalls/fcntl/Makefile
@@ -21,11 +21,14 @@ top_srcdir		?= ../../../..
 fcntl33: LDLIBS+=-lrt
 fcntl33_64: LDLIBS+=-lrt
 
-fcntl34: LDLIBS += -lpthread
-fcntl34_64: LDLIBS += -lpthread
+fcntl34: CFLAGS += -pthread
+fcntl34_64: CFLAGS += -pthread
 
-fcntl36: LDLIBS += -lpthread
-fcntl36_64: LDLIBS += -lpthread
+fcntl36: CFLAGS += -pthread
+fcntl36_64: CFLAGS += -pthread
+
+fcntl37: CFLAGS += -pthread
+fcntl37_64: CFLAGS += -pthread
 
 include $(top_srcdir)/include/mk/testcases.mk
 include $(abs_srcdir)/../utils/newer_64.mk
diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c
new file mode 100644
index 0000000..d847ae3
--- /dev/null
+++ b/testcases/kernel/syscalls/fcntl/fcntl37.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2018 Red Hat Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: "Daniel P. Berrangé" <berrange@redhat.com>
+ *
+ * This is testing for
+ *
+ *	"Record locks are not inherited by a child created via fork(2),
+ *	 but are preserved across an execve(2)."
+ *
+ * from fcntl(2) man page.
+ *
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <sched.h>
+#include <limits.h>
+
+#define TST_NO_DEFAULT_MAIN
+
+#include "lapi/fcntl.h"
+#include "tst_safe_pthread.h"
+#include "tst_test.h"
+
+static const char fname[] = "tst_lock_execve";
+
+static void *thread_fn(void *arg)
+{
+	int fd = *(intptr_t *)arg;
+	tst_res(TINFO, "Thread running. fd %d", fd);
+	while(1) sleep(1);
+	return NULL;
+}
+
+int main(int argc, char *argv[])
+{
+	int fd, ret = 0;
+
+	if (argc == 2) {
+		fd = atoi(argv[1]);
+		struct flock flck = {
+			.l_type = F_WRLCK,
+			.l_whence = SEEK_SET,
+			.l_start = 0,
+			.l_len = 1,
+		};
+		SAFE_FCNTL(fd, F_GETLK, &flck);
+		if (flck.l_type == F_UNLCK)
+			tst_res(TFAIL, "Record lock gets lost after execve");
+		else
+			tst_res(TPASS, "Record lock survives execve");
+		SAFE_CLOSE(fd);
+		SAFE_UNLINK(fname);
+		exit(0);
+	}
+
+	fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755);
+	struct flock64 flck = {
+		.l_type = F_WRLCK,
+		.l_whence = SEEK_SET,
+		.l_start  = 0,
+		.l_len    = 1,
+	};
+	SAFE_FCNTL(fd, F_SETLK, &flck);
+
+	pthread_t th;
+	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
+	sleep(1);
+
+	int flags=SAFE_FCNTL(fd, F_GETFD);
+	flags &= ~FD_CLOEXEC;
+	SAFE_FCNTL(fd, F_SETFD, flags);
+
+	char fdstr[10];
+	snprintf(fdstr, 10, "%d", fd);
+	char * const newargv[] = { argv[0], fdstr, NULL };
+	tst_res(TINFO, "execve %s with %s locked", argv[0], fname);
+	ret = execve(argv[0], newargv, NULL);
+	return ret;
+}
-- 
1.8.3.1

[LTP] [PATCH v2] fcntl37: test posix lock across execve

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
> index ae37214..7a215c8 100644
> --- a/testcases/kernel/syscalls/fcntl/Makefile
> +++ b/testcases/kernel/syscalls/fcntl/Makefile
> @@ -21,11 +21,14 @@ top_srcdir		?= ../../../..
>  fcntl33: LDLIBS+=-lrt
>  fcntl33_64: LDLIBS+=-lrt
>  
> -fcntl34: LDLIBS += -lpthread
> -fcntl34_64: LDLIBS += -lpthread
> +fcntl34: CFLAGS += -pthread
> +fcntl34_64: CFLAGS += -pthread
>  
> -fcntl36: LDLIBS += -lpthread
> -fcntl36_64: LDLIBS += -lpthread
> +fcntl36: CFLAGS += -pthread
> +fcntl36_64: CFLAGS += -pthread
> +
> +fcntl37: CFLAGS += -pthread
> +fcntl37_64: CFLAGS += -pthread

Unrelated changes (i.e. something that does not belong to the test being
added to LTP) must be put into separate patch.

>  include $(top_srcdir)/include/mk/testcases.mk
>  include $(abs_srcdir)/../utils/newer_64.mk
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c
> new file mode 100644
> index 0000000..d847ae3
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fcntl/fcntl37.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (c) 2018 Red Hat Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Author: "Daniel P. Berrang??" <berrange@redhat.com>
> + *
> + * This is testing for
> + *
> + *	"Record locks are not inherited by a child created via fork(2),
> + *	 but are preserved across an execve(2)."
> + *
> + * from fcntl(2) man page.
> + *
> + */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <pthread.h>
> +#include <sched.h>
> +#include <limits.h>
> +
> +#define TST_NO_DEFAULT_MAIN

Uh, please don't do this.

The main test process has to use the test library default main.

The helper binary, that is executed by execve(), will use this so that
the test library is not initilized twice. And the main test process will
fork before execve() because it has to stay alive. Just like in the test
I've pointed out previously.

Just have a look at the creat07.c and creat07_child.c.

> +#include "lapi/fcntl.h"
> +#include "tst_safe_pthread.h"
> +#include "tst_test.h"
> +
> +static const char fname[] = "tst_lock_execve";
> +
> +static void *thread_fn(void *arg)
> +{
> +	int fd = *(intptr_t *)arg;
                     ^
		This is still passed by a pointer not by value.


> +	tst_res(TINFO, "Thread running. fd %d", fd);
> +	while(1) sleep(1);
> +	return NULL;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int fd, ret = 0;
> +
> +	if (argc == 2) {
> +		fd = atoi(argv[1]);
> +		struct flock flck = {
> +			.l_type = F_WRLCK,
> +			.l_whence = SEEK_SET,
> +			.l_start = 0,
> +			.l_len = 1,
> +		};
> +		SAFE_FCNTL(fd, F_GETLK, &flck);
> +		if (flck.l_type == F_UNLCK)
> +			tst_res(TFAIL, "Record lock gets lost after execve");
> +		else
> +			tst_res(TPASS, "Record lock survives execve");
> +		SAFE_CLOSE(fd);
> +		SAFE_UNLINK(fname);
> +		exit(0);
> +	}
> +
> +	fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755);
> +	struct flock64 flck = {
> +		.l_type = F_WRLCK,
> +		.l_whence = SEEK_SET,
> +		.l_start  = 0,
> +		.l_len    = 1,
> +	};
> +	SAFE_FCNTL(fd, F_SETLK, &flck);
> +
> +	pthread_t th;
> +	SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd);
> +	sleep(1);

No sleep(1) in test in order to synchronize threads/processes.

> +	int flags=SAFE_FCNTL(fd, F_GETFD);
> +	flags &= ~FD_CLOEXEC;
> +	SAFE_FCNTL(fd, F_SETFD, flags);
> +
> +	char fdstr[10];
> +	snprintf(fdstr, 10, "%d", fd);
                         ^
			 sizeof(fdstr)
> +	char * const newargv[] = { argv[0], fdstr, NULL };
> +	tst_res(TINFO, "execve %s with %s locked", argv[0], fname);
> +	ret = execve(argv[0], newargv, NULL);
> +	return ret;
> +}

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fcntl37: test posix lock across execve

[Xiong Zhou]

On Mon, Mar 26, 2018 at 04:33:33PM +0200, Cyril Hrubis wrote:
> Hi!
> > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
> > index ae37214..7a06aa7 100644
> > --- a/testcases/kernel/syscalls/fcntl/Makefile
> > +++ b/testcases/kernel/syscalls/fcntl/Makefile
> > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread
> >  fcntl36: LDLIBS += -lpthread
> >  fcntl36_64: LDLIBS += -lpthread
> >  
...snip......

> > +	char prog_name[PATH_MAX];
> > +	ret = readlink("/proc/self/exe", prog_name, PATH_MAX);
> > +	char * const newargv[] = { prog_name, NULL, NULL };
> 
> This is far to complicated for no reason. The path to LTP test binaries
> is in $PATH so you can execute any LTP binary just by it's name.
> Absolute path is absolutely unneeded.

According to man 3 execve and my tests, i see absolute path is needed.

The perror after execve reports "No such file or directory".

Thanks,
Xiong

[LTP] [PATCH] fcntl37: test posix lock across execve

[Xiong Zhou]

On Mon, Mar 26, 2018 at 04:33:33PM +0200, Cyril Hrubis wrote:
> Hi!
> > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile
> > index ae37214..7a06aa7 100644
> > --- a/testcases/kernel/syscalls/fcntl/Makefile
> > +++ b/testcases/kernel/syscalls/fcntl/Makefile
> > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread
> >  fcntl36: LDLIBS += -lpthread
> >  fcntl36_64: LDLIBS += -lpthread
> >  
> > +fcntl37: LDLIBS += -lpthread
> > +fcntl37_64: LDLIBS += -lpthread
> 
> This is wrong, we should use CFLAGS += -pthread instead, also the rest
> of the Makefile should be fixed as well.
> 
> >  include $(top_srcdir)/include/mk/testcases.mk
> >  include $(abs_srcdir)/../utils/newer_64.mk
> 
> ...
> 
> > +#include "lapi/fcntl.h"
> > +#include "tst_safe_pthread.h"
> > +#include "tst_test.h"
> > +
> > +static const char fname[] = "tst_lock_execve";
> > +static const char flag_fname[] = "/tmp/execved";
>                                        ^
> 				     You must not create files in random
> 				     places. Once you set the
> 				     .needs_tmpdir in the test structure
> 				     the test is executed with unique temporary
> 				     directory as working directory so
> 				     all you need to do for temporary
> 				     files is to work with relative filenames.
> 
> > +static void cleanup(void);
> > +
> > +static void *thread_fn(void *arg)
> > +{
> > +	int fd = *(int *)arg;
>                    ^
> 		   This is broken, you have to use intptr_t
> 
> > +	tst_res(TINFO, "Thread running. fd %d", fd);
> > +	/* Just need to be alive when execve. */
> > +	sleep(5);
> > +	SAFE_CLOSE(fd);
> > +	return NULL;
> > +}
> > +
> > +static void checklock(int fd)
> > +{
> > +	pid_t pid = SAFE_FORK();
> > +	if (pid == 0) {
> > +		struct flock flck = {
> > +			.l_type = F_WRLCK,
> > +			.l_whence = SEEK_SET,
> > +			.l_start = 0,
> > +			.l_len = 1,
> > +		};
> > +		SAFE_FCNTL(fd, F_GETLK, &flck);
> > +		if (flck.l_type == F_UNLCK)
> > +			tst_res(TFAIL, "Record lock gets lost after execve");
> > +		else
> > +			tst_res(TPASS, "Record lock survives execve");
> > +		SAFE_CLOSE(fd);
> > +		_exit(0);
> 
> Why _exit() ?
> 
> > +	}
> > +	waitpid(pid, NULL, 0);
> 
> SAFE_WAITPID()
> 
> > +}
> 
> Also having the check in a separate function and calling it several
> times makes it kind of hard for debugging. if one of the assertions
> fails the error messages would be the same. Maybe we should pass a
> string with describes assertion type to the function and use it
> int the tst_res() messages.
> 
> > +static void test01(void)
> > +{
> > +	int fd, ret;
> > +	struct stat stat_buf;
> > +
> > +	/*
> > +	 * If tmp/tst_lock_execve exists, execve to run checklock.
> > +	 */
> > +	ret = stat(flag_fname, &stat_buf);
> > +	if (ret == 0) {
> > +		checklock(fd);
> > +		cleanup();
> 
> You must not call the cleanup from the test. Cleanup function is called
> from the test library.
> 
> > +		_exit(0);
> 
> Again why _exit() ?
> 
> > +	}
> 
> Don't do this.
> 
> You must not reexecute the testcase and put file flags into random
> places. If you need execve() a test helper, you have to write simple
> binary helper. See testcases/kernel/syscalls/creat/creat07_child.c

Finally get this helper working but does not reproduce the original
bug we are trying to cover. I guess the testcase re-exec is necessary
here.

If this rule is unshakeable, I'll try upstreaming it to other test
suite. Rule is rule, I respect rules.

THanks,
Xiong

[LTP] [PATCH] fcntl37: test posix lock across execve

[Cyril Hrubis]

Hi!
> Finally get this helper working but does not reproduce the original
> bug we are trying to cover. I guess the testcase re-exec is necessary
> here.

Can't we have the helper to re-exec itself?

-- 
Cyril Hrubis
chrubis@suse.cz