Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] syscalls/newmount: new test case for new mount API
@ 2019-11-28 17:35 Zorro Lang
  2019-11-28 19:14 ` Petr Vorel
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Zorro Lang @ 2019-11-28 17:35 UTC (permalink / raw)
  To: ltp; +Cc: linux-fsdevel

Linux supports new mount syscalls from 5.2, so add new test cases
to cover these new API. This newmount01 case make sure new API -
fsopen(), fsconfig(), fsmount() and move_mount() can mount a
filesystem, then can be unmounted.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Hi,

This's the 1st case for LTP to test current new mount API. So I have to add
lots of new things to include/lapi/* and m4/ltp-*(as below), I'm not familiar
with LTP code, so please help to review. There might be lot of things need to
be improved.

I'll try to add more test if this 1st case can be merged. I've tested this
patch on latest upstream xfs-linux for-next branch, due to xfs supports
the new mount API now.

# ./runltp -B xfs -f newmount
...
...
Running tests.......
<<<test_start>>>
tag=newmount01 stime=1574961655
cmdline="newmount01"
contacts=""
analysis=exit
<<<test_output>>>
incrementing stop
tst_device.c:238: INFO: Using test device LTP_DEV='/dev/loop1'
tst_test.c:1217: INFO: Timeout per run is 0h 05m 00s
tst_mkfs.c:90: INFO: Formatting /dev/loop1 with xfs opts='' extra opts=''
newmount01.c:87: PASS: fsopen xfs
newmount01.c:96: PASS: fsconfig set source to /dev/loop1
newmount01.c:105: PASS: fsconfig create superblock
newmount01.c:113: PASS: fsmount
newmount01.c:121: PASS: move_mount attach to mount point
newmount01.c:124: PASS: new mount works

Summary:
passed   6
failed   0
skipped  0
warnings 0
<<<execution_status>>>
initiation_status="ok"
duration=4 termination_type=exited termination_id=0 corefile=no
cutime=0 cstime=8
<<<test_end>>>

Thanks,
Zorro

 configure.ac                                  |   4 +
 include/lapi/newmount.h                       | 106 +++++++++++++
 include/lapi/syscalls/aarch64.in              |   4 +
 include/lapi/syscalls/powerpc64.in            |   4 +
 include/lapi/syscalls/s390x.in                |   4 +
 include/lapi/syscalls/x86_64.in               |   4 +
 m4/ltp-fsconfig.m4                            |   7 +
 m4/ltp-fsmount.m4                             |   7 +
 m4/ltp-fsopen.m4                              |   7 +
 m4/ltp-move_mount.m4                          |   7 +
 runtest/syscalls                              |   2 +
 testcases/kernel/syscalls/newmount/.gitignore |   1 +
 testcases/kernel/syscalls/newmount/Makefile   |  29 ++++
 .../kernel/syscalls/newmount/newmount01.c     | 150 ++++++++++++++++++
 14 files changed, 336 insertions(+)
 create mode 100644 include/lapi/newmount.h
 create mode 100644 m4/ltp-fsconfig.m4
 create mode 100644 m4/ltp-fsmount.m4
 create mode 100644 m4/ltp-fsopen.m4
 create mode 100644 m4/ltp-move_mount.m4
 create mode 100644 testcases/kernel/syscalls/newmount/.gitignore
 create mode 100644 testcases/kernel/syscalls/newmount/Makefile
 create mode 100644 testcases/kernel/syscalls/newmount/newmount01.c

diff --git a/configure.ac b/configure.ac
index 50d14967d..f17ab2e96 100644
--- a/configure.ac
+++ b/configure.ac
@@ -217,6 +217,9 @@ LTP_CHECK_CRYPTO
 LTP_CHECK_FANOTIFY
 LTP_CHECK_FIDEDUPE
 LTP_CHECK_FORTIFY_SOURCE
+LTP_CHECK_FSOPEN
+LTP_CHECK_FSCONFIG
+LTP_CHECK_FSMOUNT
 LTP_CHECK_FTS_H
 LTP_CHECK_IF_LINK
 LTP_CHECK_IOVEC
@@ -228,6 +231,7 @@ LTP_CHECK_LINUXRANDOM
 LTP_CHECK_MADVISE
 LTP_CHECK_MKDTEMP
 LTP_CHECK_MMSGHDR
+LTP_CHECK_MOVE_MOUNT
 LTP_CHECK_MREMAP_FIXED
 LTP_CHECK_NOMMU_LINUX
 LTP_CHECK_PERF_EVENT
diff --git a/include/lapi/newmount.h b/include/lapi/newmount.h
new file mode 100644
index 000000000..07d57ff96
--- /dev/null
+++ b/include/lapi/newmount.h
@@ -0,0 +1,106 @@
+/*
+ * Copyright (C) 2019 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, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#ifndef __NEWMOUNT_H__
+#define __NEWMOUNT_H__
+
+#include <stdint.h>
+#include <unistd.h>
+#include "lapi/syscalls.h"
+
+#if !defined(HAVE_FSOPEN)
+static inline int fsopen(const char *fs_name, unsigned int flags)
+{
+	return tst_syscall(__NR_fsopen, fs_name, flags);
+}
+
+/*
+ * fsopen() flags.
+ */
+#define FSOPEN_CLOEXEC		0x00000001
+#endif
+
+#if !defined(HAVE_FSCONFIG)
+static inline int fsconfig(int fsfd, unsigned int cmd,
+                           const char *key, const void *val, int aux)
+{
+	return tst_syscall(__NR_fsconfig, fsfd, cmd, key, val, aux);
+}
+
+/*
+ * The type of fsconfig() call made.
+ */
+enum fsconfig_command {
+	FSCONFIG_SET_FLAG	= 0,    /* Set parameter, supplying no value */
+	FSCONFIG_SET_STRING	= 1,    /* Set parameter, supplying a string value */
+	FSCONFIG_SET_BINARY	= 2,    /* Set parameter, supplying a binary blob value */
+	FSCONFIG_SET_PATH	= 3,    /* Set parameter, supplying an object by path */
+	FSCONFIG_SET_PATH_EMPTY	= 4,    /* Set parameter, supplying an object by (empty) path */
+	FSCONFIG_SET_FD		= 5,    /* Set parameter, supplying an object by fd */
+	FSCONFIG_CMD_CREATE	= 6,    /* Invoke superblock creation */
+	FSCONFIG_CMD_RECONFIGURE = 7,   /* Invoke superblock reconfiguration */
+};
+#endif
+
+#if !defined(HAVE_FSMOUNT)
+static inline int fsmount(int fsfd, unsigned int flags, unsigned int ms_flags)
+{
+	return tst_syscall(__NR_fsmount, fsfd, flags, ms_flags);
+}
+
+/*
+ * fsmount() flags.
+ */
+#define FSMOUNT_CLOEXEC		0x00000001
+
+/*
+ * Mount attributes.
+ */
+#define MOUNT_ATTR_RDONLY	0x00000001 /* Mount read-only */
+#define MOUNT_ATTR_NOSUID	0x00000002 /* Ignore suid and sgid bits */
+#define MOUNT_ATTR_NODEV	0x00000004 /* Disallow access to device special files */
+#define MOUNT_ATTR_NOEXEC	0x00000008 /* Disallow program execution */
+#define MOUNT_ATTR__ATIME	0x00000070 /* Setting on how atime should be updated */
+#define MOUNT_ATTR_RELATIME	0x00000000 /* - Update atime relative to mtime/ctime. */
+#define MOUNT_ATTR_NOATIME	0x00000010 /* - Do not update access times. */
+#define MOUNT_ATTR_STRICTATIME	0x00000020 /* - Always perform atime updates */
+#define MOUNT_ATTR_NODIRATIME	0x00000080 /* Do not update directory access times */
+#endif
+
+#if !defined(HAVE_MOVE_MOUNT)
+static inline int move_mount(int from_dfd, const char *from_pathname,
+                             int to_dfd, const char *to_pathname,
+                             unsigned int flags)
+{
+	return tst_syscall(__NR_move_mount, from_dfd, from_pathname, to_dfd,
+	                   to_pathname, flags);
+}
+
+/*
+ * move_mount() flags.
+ */
+#define MOVE_MOUNT_F_SYMLINKS		0x00000001 /* Follow symlinks on from path */
+#define MOVE_MOUNT_F_AUTOMOUNTS		0x00000002 /* Follow automounts on from path */
+#define MOVE_MOUNT_F_EMPTY_PATH		0x00000004 /* Empty from path permitted */
+#define MOVE_MOUNT_T_SYMLINKS		0x00000010 /* Follow symlinks on to path */
+#define MOVE_MOUNT_T_AUTOMOUNTS		0x00000020 /* Follow automounts on to path */
+#define MOVE_MOUNT_T_EMPTY_PATH		0x00000040 /* Empty to path permitted */
+#define MOVE_MOUNT__MASK		0x00000077
+#endif
+
+#endif /* __NEWMOUNT_H__ */
diff --git a/include/lapi/syscalls/aarch64.in b/include/lapi/syscalls/aarch64.in
index 0e00641bc..5b9e1d9a4 100644
--- a/include/lapi/syscalls/aarch64.in
+++ b/include/lapi/syscalls/aarch64.in
@@ -270,4 +270,8 @@ pkey_mprotect 288
 pkey_alloc 289
 pkey_free 290
 pidfd_send_signal 424
+move_mount 429
+fsopen 430
+fsconfig 431
+fsmount 432
 _sysctl 1078
diff --git a/include/lapi/syscalls/powerpc64.in b/include/lapi/syscalls/powerpc64.in
index 660165d7a..3aaed64e0 100644
--- a/include/lapi/syscalls/powerpc64.in
+++ b/include/lapi/syscalls/powerpc64.in
@@ -359,3 +359,7 @@ pidfd_send_signal 424
 pkey_mprotect 386
 pkey_alloc 384
 pkey_free 385
+move_mount 429
+fsopen 430
+fsconfig 431
+fsmount 432
diff --git a/include/lapi/syscalls/s390x.in b/include/lapi/syscalls/s390x.in
index 7d632d1dc..bd427555a 100644
--- a/include/lapi/syscalls/s390x.in
+++ b/include/lapi/syscalls/s390x.in
@@ -341,3 +341,7 @@ pkey_mprotect 384
 pkey_alloc 385
 pkey_free 386
 pidfd_send_signal 424
+move_mount 429
+fsopen 430
+fsconfig 431
+fsmount 432
diff --git a/include/lapi/syscalls/x86_64.in b/include/lapi/syscalls/x86_64.in
index b1cbd4f2f..94f0b562e 100644
--- a/include/lapi/syscalls/x86_64.in
+++ b/include/lapi/syscalls/x86_64.in
@@ -320,3 +320,7 @@ pkey_alloc 330
 pkey_free 331
 statx 332
 pidfd_send_signal 424
+move_mount 429
+fsopen 430
+fsconfig 431
+fsmount 432
diff --git a/m4/ltp-fsconfig.m4 b/m4/ltp-fsconfig.m4
new file mode 100644
index 000000000..397027f1b
--- /dev/null
+++ b/m4/ltp-fsconfig.m4
@@ -0,0 +1,7 @@
+dnl SPDX-License-Identifier: GPL-2.0-or-later
+dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
+
+AC_DEFUN([LTP_CHECK_FSCONFIG],[
+AC_CHECK_FUNCS(fsconfig,,)
+AC_CHECK_HEADER(sys/mount.h,,,)
+])
diff --git a/m4/ltp-fsmount.m4 b/m4/ltp-fsmount.m4
new file mode 100644
index 000000000..ee32ef713
--- /dev/null
+++ b/m4/ltp-fsmount.m4
@@ -0,0 +1,7 @@
+dnl SPDX-License-Identifier: GPL-2.0-or-later
+dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
+
+AC_DEFUN([LTP_CHECK_FSMOUNT],[
+AC_CHECK_FUNCS(fsmount,,)
+AC_CHECK_HEADER(sys/mount.h,,,)
+])
diff --git a/m4/ltp-fsopen.m4 b/m4/ltp-fsopen.m4
new file mode 100644
index 000000000..6e23d437d
--- /dev/null
+++ b/m4/ltp-fsopen.m4
@@ -0,0 +1,7 @@
+dnl SPDX-License-Identifier: GPL-2.0-or-later
+dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
+
+AC_DEFUN([LTP_CHECK_FSOPEN],[
+AC_CHECK_FUNCS(fsopen,,)
+AC_CHECK_HEADER(sys/mount.h,,,)
+])
diff --git a/m4/ltp-move_mount.m4 b/m4/ltp-move_mount.m4
new file mode 100644
index 000000000..d6bfd82e9
--- /dev/null
+++ b/m4/ltp-move_mount.m4
@@ -0,0 +1,7 @@
+dnl SPDX-License-Identifier: GPL-2.0-or-later
+dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
+
+AC_DEFUN([LTP_CHECK_MOVE_MOUNT],[
+AC_CHECK_FUNCS(move_mount,,)
+AC_CHECK_HEADER(sys/mount.h,,,)
+])
diff --git a/runtest/syscalls b/runtest/syscalls
index 15dbd9971..d11a87dd9 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -716,6 +716,8 @@ mount04 mount04
 mount05 mount05
 mount06 mount06
 
+newmount01 newmount01
+
 move_pages01 move_pages01
 move_pages02 move_pages02
 move_pages03 move_pages03
diff --git a/testcases/kernel/syscalls/newmount/.gitignore b/testcases/kernel/syscalls/newmount/.gitignore
new file mode 100644
index 000000000..dc78edd5b
--- /dev/null
+++ b/testcases/kernel/syscalls/newmount/.gitignore
@@ -0,0 +1 @@
+/newmount01
diff --git a/testcases/kernel/syscalls/newmount/Makefile b/testcases/kernel/syscalls/newmount/Makefile
new file mode 100644
index 000000000..8b0a60332
--- /dev/null
+++ b/testcases/kernel/syscalls/newmount/Makefile
@@ -0,0 +1,29 @@
+#
+#  Copyright (C) 2017 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 will 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, write to the Free Software
+#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+# HISTORY:
+#  27/11/2019 zlang@redhat.com  Create newmount01.c
+#
+#############################################################################
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+CFLAGS			+= -D_GNU_SOURCE
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/newmount/newmount01.c b/testcases/kernel/syscalls/newmount/newmount01.c
new file mode 100644
index 000000000..35e355506
--- /dev/null
+++ b/testcases/kernel/syscalls/newmount/newmount01.c
@@ -0,0 +1,150 @@
+/*
+ * Copyright (C) 2019 Red Hat, Inc.  All rights reserved.
+ * Author: Zorro Lang <zlang@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+/*
+ *  DESCRIPTION
+ *	Use new mount API (fsopen, fsconfig, fsmount, move_mount) to mount
+ *	a filesystem.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <sys/mount.h>
+
+#include "tst_test.h"
+#include "tst_safe_macros.h"
+#include "lapi/newmount.h"
+
+#define LINELENGTH 256
+#define MNTPOINT "newmount_point"
+static int sfd, mfd;
+static int mount_flag = 0;
+
+static int ismount(char *mntpoint)
+{
+	int ret = 0;
+	FILE *file;
+	char line[LINELENGTH];
+
+	file = fopen("/proc/mounts", "r");
+	if (file == NULL)
+		tst_brk(TFAIL | TTERRNO, "Open /proc/mounts failed");
+
+	while (fgets(line, LINELENGTH, file) != NULL) {
+		if (strstr(line, mntpoint) != NULL) {
+			ret = 1;
+			break;
+		}
+	}
+	fclose(file);
+	return ret;
+}
+
+static void setup(void)
+{
+	SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL);
+}
+
+static void cleanup(void)
+{
+	if (mount_flag == 1) {
+		TEST(tst_umount(MNTPOINT));
+		if (TST_RET != 0)
+			tst_brk(TBROK | TTERRNO, "umount failed");
+	}
+}
+
+
+static void test_newmount(void)
+{
+	TEST(fsopen(tst_device->fs_type, FSOPEN_CLOEXEC));
+	if (TST_RET < 0) {
+		tst_brk(TFAIL | TTERRNO,
+		        "fsopen %s", tst_device->fs_type);
+	} else {
+		sfd = TST_RET;
+		tst_res(TPASS,
+			"fsopen %s", tst_device->fs_type);
+	}
+
+	TEST(fsconfig(sfd, FSCONFIG_SET_STRING, "source", tst_device->dev, 0));
+	if (TST_RET < 0) {
+		tst_brk(TFAIL | TTERRNO,
+		        "fsconfig set source to %s", tst_device->dev);
+	} else {
+		tst_res(TPASS,
+			"fsconfig set source to %s", tst_device->dev);
+	}
+
+	TEST(fsconfig(sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0));
+	if (TST_RET < 0) {
+		tst_brk(TFAIL | TTERRNO,
+		        "fsconfig create superblock");
+	} else {
+		tst_res(TPASS,
+			"fsconfig create superblock");
+	}
+
+	TEST(fsmount(sfd, FSMOUNT_CLOEXEC, 0));
+	if (TST_RET < 0) {
+		tst_brk(TFAIL | TTERRNO, "fsmount");
+	} else {
+		mfd = TST_RET;
+		tst_res(TPASS, "fsmount");
+		SAFE_CLOSE(sfd);
+	}
+
+	TEST(move_mount(mfd, "", AT_FDCWD, MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH));
+	if (TST_RET < 0) {
+		tst_brk(TFAIL | TTERRNO, "move_mount attach to mount point");
+	} else {
+		tst_res(TPASS, "move_mount attach to mount point");
+		mount_flag = 1;
+		if (ismount(MNTPOINT))
+			tst_res(TPASS, "new mount works");
+		else
+			tst_res(TFAIL, "new mount fails");
+	}
+	SAFE_CLOSE(mfd);
+}
+
+struct test_cases {
+	void (*tfunc)(void);
+} tcases[] = {
+	{&test_newmount},
+};
+
+static void run(unsigned int i)
+{
+	tcases[i].tfunc();
+}
+
+static struct tst_test test = {
+	.test		= run,
+	.tcnt		= ARRAY_SIZE(tcases),
+	.setup		= setup,
+	.cleanup	= cleanup,
+	.needs_root	= 1,
+	.mntpoint	= MNTPOINT,
+	.needs_device	= 1,
+};
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] syscalls/newmount: new test case for new mount API
  2019-11-28 17:35 [PATCH] syscalls/newmount: new test case for new mount API Zorro Lang
@ 2019-11-28 19:14 ` Petr Vorel
  2019-11-29  3:48   ` Zorro Lang
       [not found] ` <9c487d75-0de0-af8f-a439-d3ce9d965808@cn.fujitsu.com>
  2019-12-03 13:03 ` Cyril Hrubis
  2 siblings, 1 reply; 8+ messages in thread
From: Petr Vorel @ 2019-11-28 19:14 UTC (permalink / raw)
  To: Zorro Lang; +Cc: ltp, linux-fsdevel

Hi Zorro,

> Linux supports new mount syscalls from 5.2, so add new test cases
> to cover these new API. This newmount01 case make sure new API -
> fsopen(), fsconfig(), fsmount() and move_mount() can mount a
> filesystem, then can be unmounted.
Thanks for writing test for recently added kernel functionality.
This is important.
Test itself looks ok to me.
There are few code style differences (note below), but that's not important.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW I thought it'd be nice to use more filesystems via .all_filesystems = 1 [1]
but at least it breaks nfs. And IMHO we don't have blacklist support for
.all_filesystems.

>  configure.ac                                  |   4 +
>  include/lapi/newmount.h                       | 106 +++++++++++++
>  include/lapi/syscalls/aarch64.in              |   4 +
>  include/lapi/syscalls/powerpc64.in            |   4 +
>  include/lapi/syscalls/s390x.in                |   4 +
>  include/lapi/syscalls/x86_64.in               |   4 +
In final version we'd want to add syscall numbers for all archs.

...
> +++ b/include/lapi/newmount.h
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright (C) 2019 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, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
Use SPDX license identifier instead of verbose GPL everywhere (including headers
and makefiles; we don't want any HISTORY: text, but feel free to add Author:
your name).
> +
> +#ifndef __NEWMOUNT_H__
> +#define __NEWMOUNT_H__
Double underscore at the beginning and end (__FOO_H__) is IMHO reserved for library
(use NEWMOUNT_H__).
...

> diff --git a/m4/ltp-fsconfig.m4 b/m4/ltp-fsconfig.m4
> new file mode 100644
> index 000000000..397027f1b
> --- /dev/null
> +++ b/m4/ltp-fsconfig.m4
> @@ -0,0 +1,7 @@
> +dnl SPDX-License-Identifier: GPL-2.0-or-later
> +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> +
> +AC_DEFUN([LTP_CHECK_FSCONFIG],[
> +AC_CHECK_FUNCS(fsconfig,,)
> +AC_CHECK_HEADER(sys/mount.h,,,)
> +])
> diff --git a/m4/ltp-fsmount.m4 b/m4/ltp-fsmount.m4
> new file mode 100644
> index 000000000..ee32ef713
> --- /dev/null
> +++ b/m4/ltp-fsmount.m4
> @@ -0,0 +1,7 @@
> +dnl SPDX-License-Identifier: GPL-2.0-or-later
> +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> +
> +AC_DEFUN([LTP_CHECK_FSMOUNT],[
> +AC_CHECK_FUNCS(fsmount,,)
> +AC_CHECK_HEADER(sys/mount.h,,,)
> +])
> diff --git a/m4/ltp-fsopen.m4 b/m4/ltp-fsopen.m4
> new file mode 100644
> index 000000000..6e23d437d
> --- /dev/null
> +++ b/m4/ltp-fsopen.m4
> @@ -0,0 +1,7 @@
> +dnl SPDX-License-Identifier: GPL-2.0-or-later
> +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> +
> +AC_DEFUN([LTP_CHECK_FSOPEN],[
> +AC_CHECK_FUNCS(fsopen,,)
> +AC_CHECK_HEADER(sys/mount.h,,,)
> +])
> diff --git a/m4/ltp-move_mount.m4 b/m4/ltp-move_mount.m4
> new file mode 100644
> index 000000000..d6bfd82e9
> --- /dev/null
> +++ b/m4/ltp-move_mount.m4
> @@ -0,0 +1,7 @@
> +dnl SPDX-License-Identifier: GPL-2.0-or-later
> +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> +
> +AC_DEFUN([LTP_CHECK_MOVE_MOUNT],[
> +AC_CHECK_FUNCS(move_mount,,)
> +AC_CHECK_HEADER(sys/mount.h,,,)
> +])
As all of these require <sys/mount.h>, I'd add them into single file
m4/ltp-newmount.m4.
BTW it might take a time before it get into <sys/mount.h>, they're now just <linux/mount.h> (even in musl, which is unlike glic fast with porting new things).

...
> +++ b/testcases/kernel/syscalls/newmount/Makefile
...
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +
> +CFLAGS			+= -D_GNU_SOURCE
Is _GNU_SOURCE needed?
> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2215-testing-with-a-block-device

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] syscalls/newmount: new test case for new mount API
  2019-11-28 19:14 ` Petr Vorel
@ 2019-11-29  3:48   ` Zorro Lang
  0 siblings, 0 replies; 8+ messages in thread
From: Zorro Lang @ 2019-11-29  3:48 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp, linux-fsdevel

On Thu, Nov 28, 2019 at 08:14:42PM +0100, Petr Vorel wrote:
> Hi Zorro,
> 
> > Linux supports new mount syscalls from 5.2, so add new test cases
> > to cover these new API. This newmount01 case make sure new API -
> > fsopen(), fsconfig(), fsmount() and move_mount() can mount a
> > filesystem, then can be unmounted.
> Thanks for writing test for recently added kernel functionality.
> This is important.
> Test itself looks ok to me.
> There are few code style differences (note below), but that's not important.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> BTW I thought it'd be nice to use more filesystems via .all_filesystems = 1 [1]
> but at least it breaks nfs. And IMHO we don't have blacklist support for
> .all_filesystems.

I(or with my colleagues) would like to add more filesystem specified test later,
to make sure filesystem specified mount options still works well with new mount
syscalls.

> 
> >  configure.ac                                  |   4 +
> >  include/lapi/newmount.h                       | 106 +++++++++++++
> >  include/lapi/syscalls/aarch64.in              |   4 +
> >  include/lapi/syscalls/powerpc64.in            |   4 +
> >  include/lapi/syscalls/s390x.in                |   4 +
> >  include/lapi/syscalls/x86_64.in               |   4 +
> In final version we'd want to add syscall numbers for all archs.

Yeah, I tried to find a .in file for all archs, but didn't find, so had to
add these __NR_ definition separately.

> 
> ...
> > +++ b/include/lapi/newmount.h
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Copyright (C) 2019 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, write the Free Software Foundation,
> > + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + */
> Use SPDX license identifier instead of verbose GPL everywhere (including headers
> and makefiles; we don't want any HISTORY: text, but feel free to add Author:
> your name).

Wow, sorry I don't learn about the license things so much, just copy from other file:)
I'll search how to use the SPDX license.

> > +
> > +#ifndef __NEWMOUNT_H__
> > +#define __NEWMOUNT_H__
> Double underscore at the beginning and end (__FOO_H__) is IMHO reserved for library
> (use NEWMOUNT_H__).

Sure, I'll change it to a proper one.

> ...
> 
> > diff --git a/m4/ltp-fsconfig.m4 b/m4/ltp-fsconfig.m4
> > new file mode 100644
> > index 000000000..397027f1b
> > --- /dev/null
> > +++ b/m4/ltp-fsconfig.m4
> > @@ -0,0 +1,7 @@
> > +dnl SPDX-License-Identifier: GPL-2.0-or-later
> > +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> > +
> > +AC_DEFUN([LTP_CHECK_FSCONFIG],[
> > +AC_CHECK_FUNCS(fsconfig,,)
> > +AC_CHECK_HEADER(sys/mount.h,,,)
> > +])
> > diff --git a/m4/ltp-fsmount.m4 b/m4/ltp-fsmount.m4
> > new file mode 100644
> > index 000000000..ee32ef713
> > --- /dev/null
> > +++ b/m4/ltp-fsmount.m4
> > @@ -0,0 +1,7 @@
> > +dnl SPDX-License-Identifier: GPL-2.0-or-later
> > +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> > +
> > +AC_DEFUN([LTP_CHECK_FSMOUNT],[
> > +AC_CHECK_FUNCS(fsmount,,)
> > +AC_CHECK_HEADER(sys/mount.h,,,)
> > +])
> > diff --git a/m4/ltp-fsopen.m4 b/m4/ltp-fsopen.m4
> > new file mode 100644
> > index 000000000..6e23d437d
> > --- /dev/null
> > +++ b/m4/ltp-fsopen.m4
> > @@ -0,0 +1,7 @@
> > +dnl SPDX-License-Identifier: GPL-2.0-or-later
> > +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> > +
> > +AC_DEFUN([LTP_CHECK_FSOPEN],[
> > +AC_CHECK_FUNCS(fsopen,,)
> > +AC_CHECK_HEADER(sys/mount.h,,,)
> > +])
> > diff --git a/m4/ltp-move_mount.m4 b/m4/ltp-move_mount.m4
> > new file mode 100644
> > index 000000000..d6bfd82e9
> > --- /dev/null
> > +++ b/m4/ltp-move_mount.m4
> > @@ -0,0 +1,7 @@
> > +dnl SPDX-License-Identifier: GPL-2.0-or-later
> > +dnl Copyright (C) 2019 Red Hat, Inc. All Rights Reserved.
> > +
> > +AC_DEFUN([LTP_CHECK_MOVE_MOUNT],[
> > +AC_CHECK_FUNCS(move_mount,,)
> > +AC_CHECK_HEADER(sys/mount.h,,,)
> > +])
> As all of these require <sys/mount.h>, I'd add them into single file
> m4/ltp-newmount.m4.

OK, I'll do that.

> BTW it might take a time before it get into <sys/mount.h>, they're now just <linux/mount.h> (even in musl, which is unlike glic fast with porting new things).

Yes, there're still in kernel-headers, glibc doesn't have patch for that. Maybe
they're waiting. I don't know if there'll be more newmount syscalls (e.g fsinfo
or something else), or fsdevel might would like to disconnect umount() in the
feature:)

> 
> ...
> > +++ b/testcases/kernel/syscalls/newmount/Makefile
> ...
> > +
> > +top_srcdir		?= ../../../..
> > +
> > +include $(top_srcdir)/include/mk/testcases.mk
> > +
> > +CFLAGS			+= -D_GNU_SOURCE
> Is _GNU_SOURCE needed?

Hmm... I'm not sure, just copy this Makefile from syscalls/mount/Makefile ;)
I think the new mount API might not be POSIX defined?

Thanks for your review so much, I'll send V2 patch soon.

Thanks,
Zorro

> > +
> > +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> 
> Kind regards,
> Petr
> 
> [1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2215-testing-with-a-block-device
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [LTP] [PATCH] syscalls/newmount: new test case for new mount API
       [not found] ` <9c487d75-0de0-af8f-a439-d3ce9d965808@cn.fujitsu.com>
@ 2019-11-29  5:29   ` " Yang Xu
  2019-11-29 11:20     ` Zorro Lang
  0 siblings, 1 reply; 8+ messages in thread
From: Yang Xu @ 2019-11-29  5:29 UTC (permalink / raw)
  To: Zorro Lang, ltp; +Cc: linux-fsdevel



on 2019/11/29 11:39, Yang Xu wrote:
> --- /dev/null
> +++ b/include/lapi/newmount.h
> @@ -0,0 +1,106 @@
> +/*
> + * Copyright (C) 2019 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, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#ifndef __NEWMOUNT_H__
> +#define __NEWMOUNT_H__
> +
> +#include <stdint.h>
> +#include <unistd.h>
> +#include "lapi/syscalls.h"
> +
> +#if !defined(HAVE_FSOPEN)
When we run make autotools and ./configure, this macro is in 
"include/config.h". You should include this header file like other lapi 
files.
> +static inline int fsopen(const char *fs_name, unsigned int flags)
> +{



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [LTP] [PATCH] syscalls/newmount: new test case for new mount API
  2019-11-29  5:29   ` [LTP] " Yang Xu
@ 2019-11-29 11:20     ` Zorro Lang
  0 siblings, 0 replies; 8+ messages in thread
From: Zorro Lang @ 2019-11-29 11:20 UTC (permalink / raw)
  To: Yang Xu; +Cc: ltp, linux-fsdevel

On Fri, Nov 29, 2019 at 01:29:35PM +0800, Yang Xu wrote:
> 
> 
> on 2019/11/29 11:39, Yang Xu wrote:
> > --- /dev/null
> > +++ b/include/lapi/newmount.h
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Copyright (C) 2019 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, write the Free Software Foundation,
> > + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + */
> > +
> > +#ifndef __NEWMOUNT_H__
> > +#define __NEWMOUNT_H__
> > +
> > +#include <stdint.h>
> > +#include <unistd.h>
> > +#include "lapi/syscalls.h"
> > +
> > +#if !defined(HAVE_FSOPEN)
> When we run make autotools and ./configure, this macro is in
> "include/config.h". You should include this header file like other lapi
> files.

Oh, thanks, I refered to the include/lapi/stat.h file, it doesn't include
config.h, I don't know if it's needed.

Thanks,
Zorro

> > +static inline int fsopen(const char *fs_name, unsigned int flags)
> > +{
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [LTP] [PATCH] syscalls/newmount: new test case for new mount API
  2019-11-28 17:35 [PATCH] syscalls/newmount: new test case for new mount API Zorro Lang
  2019-11-28 19:14 ` Petr Vorel
       [not found] ` <9c487d75-0de0-af8f-a439-d3ce9d965808@cn.fujitsu.com>
@ 2019-12-03 13:03 ` Cyril Hrubis
  2019-12-06 16:23   ` Zorro Lang
  2 siblings, 1 reply; 8+ messages in thread
From: Cyril Hrubis @ 2019-12-03 13:03 UTC (permalink / raw)
  To: Zorro Lang; +Cc: ltp, linux-fsdevel

Hi!
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/newmount/newmount01.c b/testcases/kernel/syscalls/newmount/newmount01.c
> new file mode 100644
> index 000000000..35e355506
> --- /dev/null
> +++ b/testcases/kernel/syscalls/newmount/newmount01.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright (C) 2019 Red Hat, Inc.  All rights reserved.
> + * Author: Zorro Lang <zlang@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +/*
> + *  DESCRIPTION
> + *	Use new mount API (fsopen, fsconfig, fsmount, move_mount) to mount
> + *	a filesystem.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <sys/prctl.h>
> +#include <sys/wait.h>
> +#include <sys/mount.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +#include "lapi/newmount.h"
> +
> +#define LINELENGTH 256
> +#define MNTPOINT "newmount_point"
> +static int sfd, mfd;
> +static int mount_flag = 0;
> +
> +static int ismount(char *mntpoint)
> +{
> +	int ret = 0;
> +	FILE *file;
> +	char line[LINELENGTH];
> +
> +	file = fopen("/proc/mounts", "r");
> +	if (file == NULL)
> +		tst_brk(TFAIL | TTERRNO, "Open /proc/mounts failed");
> +
> +	while (fgets(line, LINELENGTH, file) != NULL) {
> +		if (strstr(line, mntpoint) != NULL) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +	fclose(file);
> +	return ret;
> +}

Hmm, this is very similar to file_lines_scanf(), maybe we need a library
function that would iterate over file lines to call a callback on each
of them as well. I will think about this.

> +static void setup(void)
> +{
> +	SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL);

Why aren't we just setting .format_device in the test structure?

> +}
> +
> +static void cleanup(void)
> +{
> +	if (mount_flag == 1) {
> +		TEST(tst_umount(MNTPOINT));
> +		if (TST_RET != 0)
> +			tst_brk(TBROK | TTERRNO, "umount failed");

The library already produces TWARN if we fail to umount the device, so I
would say that there is no need to TBROK here, the TBROK will be
converted to TWARN anyways since it's in the cleanup...

> +	}
> +}
> +
> +
> +static void test_newmount(void)
> +{
> +	TEST(fsopen(tst_device->fs_type, FSOPEN_CLOEXEC));
> +	if (TST_RET < 0) {
> +		tst_brk(TFAIL | TTERRNO,
> +		        "fsopen %s", tst_device->fs_type);
> +	} else {

There is no need for else branches after tst_brk(), the test will exit
if we reach the tst_brk().

> +		sfd = TST_RET;
> +		tst_res(TPASS,
> +			"fsopen %s", tst_device->fs_type);
> +	}
> +
> +	TEST(fsconfig(sfd, FSCONFIG_SET_STRING, "source", tst_device->dev, 0));
> +	if (TST_RET < 0) {
> +		tst_brk(TFAIL | TTERRNO,
> +		        "fsconfig set source to %s", tst_device->dev);
> +	} else {

Here as well.

> +		tst_res(TPASS,
> +			"fsconfig set source to %s", tst_device->dev);
> +	}
> +
> +	TEST(fsconfig(sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0));
> +	if (TST_RET < 0) {
> +		tst_brk(TFAIL | TTERRNO,
> +		        "fsconfig create superblock");

And here.

> +	} else {
> +		tst_res(TPASS,
> +			"fsconfig create superblock");
> +	}
> +
> +	TEST(fsmount(sfd, FSMOUNT_CLOEXEC, 0));
> +	if (TST_RET < 0) {
> +		tst_brk(TFAIL | TTERRNO, "fsmount");
> +	} else {

And here.

> +		mfd = TST_RET;
> +		tst_res(TPASS, "fsmount");
> +		SAFE_CLOSE(sfd);
> +	}
> +
> +	TEST(move_mount(mfd, "", AT_FDCWD, MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH));
> +	if (TST_RET < 0) {
> +		tst_brk(TFAIL | TTERRNO, "move_mount attach to mount point");
> +	} else {

And here.

> +		tst_res(TPASS, "move_mount attach to mount point");
> +		mount_flag = 1;
> +		if (ismount(MNTPOINT))
> +			tst_res(TPASS, "new mount works");
> +		else
> +			tst_res(TFAIL, "new mount fails");
> +	}
> +	SAFE_CLOSE(mfd);

We have to umount the device here, otherwise it would be mounted for
each test iteration with -i.

> +}
> +
> +struct test_cases {
> +	void (*tfunc)(void);
> +} tcases[] = {
> +	{&test_newmount},
> +};

Unless you plan to add more tests here, there is no point in declaring
the structure with function pointers.

> +static void run(unsigned int i)
> +{
> +	tcases[i].tfunc();
> +}
> +
> +static struct tst_test test = {
> +	.test		= run,
> +	.tcnt		= ARRAY_SIZE(tcases),
> +	.setup		= setup,
> +	.cleanup	= cleanup,
> +	.needs_root	= 1,
> +	.mntpoint	= MNTPOINT,
> +	.needs_device	= 1,
> +};

Otherwise it looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [LTP] [PATCH] syscalls/newmount: new test case for new mount API
  2019-12-06 16:23   ` Zorro Lang
@ 2019-12-06 16:18     ` Cyril Hrubis
  0 siblings, 0 replies; 8+ messages in thread
From: Cyril Hrubis @ 2019-12-06 16:18 UTC (permalink / raw)
  To: ltp, linux-fsdevel

Hi!
> Sorry I can't be 100% sure what you mean at here. Do you mean as this:
> --
> TEST(fsopen(tst_device->fs_type, FSOPEN_CLOEXEC));
> if (TST_RET < 0) {
> 	tst_brk(TFAIL | TTERRNO,
> 		"fsopen %s", tst_device->fs_type);
> }
> sfd = TST_RET;
> tst_res(TPASS, "fsopen %s", tst_device->fs_type);

Yes, indeed. The tst_brk() calls exit() so it never returns back to the
caller.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [LTP] [PATCH] syscalls/newmount: new test case for new mount API
  2019-12-03 13:03 ` Cyril Hrubis
@ 2019-12-06 16:23   ` Zorro Lang
  2019-12-06 16:18     ` Cyril Hrubis
  0 siblings, 1 reply; 8+ messages in thread
From: Zorro Lang @ 2019-12-06 16:23 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: ltp, linux-fsdevel

On Tue, Dec 03, 2019 at 02:03:39PM +0100, Cyril Hrubis wrote:
> Hi!
> > +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> > diff --git a/testcases/kernel/syscalls/newmount/newmount01.c b/testcases/kernel/syscalls/newmount/newmount01.c
> > new file mode 100644
> > index 000000000..35e355506
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/newmount/newmount01.c
> > @@ -0,0 +1,150 @@
> > +/*
> > + * Copyright (C) 2019 Red Hat, Inc.  All rights reserved.
> > + * Author: Zorro Lang <zlang@redhat.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write the Free Software Foundation, Inc.,
> > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > + *
> > + */
> > +
> > +/*
> > + *  DESCRIPTION
> > + *	Use new mount API (fsopen, fsconfig, fsmount, move_mount) to mount
> > + *	a filesystem.
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <sys/prctl.h>
> > +#include <sys/wait.h>
> > +#include <sys/mount.h>
> > +
> > +#include "tst_test.h"
> > +#include "tst_safe_macros.h"
> > +#include "lapi/newmount.h"
> > +
> > +#define LINELENGTH 256
> > +#define MNTPOINT "newmount_point"
> > +static int sfd, mfd;
> > +static int mount_flag = 0;
> > +
> > +static int ismount(char *mntpoint)
> > +{
> > +	int ret = 0;
> > +	FILE *file;
> > +	char line[LINELENGTH];
> > +
> > +	file = fopen("/proc/mounts", "r");
> > +	if (file == NULL)
> > +		tst_brk(TFAIL | TTERRNO, "Open /proc/mounts failed");
> > +
> > +	while (fgets(line, LINELENGTH, file) != NULL) {
> > +		if (strstr(line, mntpoint) != NULL) {
> > +			ret = 1;
> > +			break;
> > +		}
> > +	}
> > +	fclose(file);
> > +	return ret;
> > +}
> 
> Hmm, this is very similar to file_lines_scanf(), maybe we need a library
> function that would iterate over file lines to call a callback on each
> of them as well. I will think about this.
> 
> > +static void setup(void)
> > +{
> > +	SAFE_MKFS(tst_device->dev, tst_device->fs_type, NULL, NULL);
> 
> Why aren't we just setting .format_device in the test structure?
> 
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +	if (mount_flag == 1) {
> > +		TEST(tst_umount(MNTPOINT));
> > +		if (TST_RET != 0)
> > +			tst_brk(TBROK | TTERRNO, "umount failed");
> 
> The library already produces TWARN if we fail to umount the device, so I
> would say that there is no need to TBROK here, the TBROK will be
> converted to TWARN anyways since it's in the cleanup...
> 
> > +	}
> > +}
> > +
> > +
> > +static void test_newmount(void)
> > +{
> > +	TEST(fsopen(tst_device->fs_type, FSOPEN_CLOEXEC));
> > +	if (TST_RET < 0) {
> > +		tst_brk(TFAIL | TTERRNO,
> > +		        "fsopen %s", tst_device->fs_type);
> > +	} else {
> 
> There is no need for else branches after tst_brk(), the test will exit
> if we reach the tst_brk().

Sorry I can't be 100% sure what you mean at here. Do you mean as this:
--
TEST(fsopen(tst_device->fs_type, FSOPEN_CLOEXEC));
if (TST_RET < 0) {
	tst_brk(TFAIL | TTERRNO,
		"fsopen %s", tst_device->fs_type);
}
sfd = TST_RET;
tst_res(TPASS, "fsopen %s", tst_device->fs_type);
--

> 
> > +		sfd = TST_RET;
> > +		tst_res(TPASS,
> > +			"fsopen %s", tst_device->fs_type);
> > +	}
> > +
> > +	TEST(fsconfig(sfd, FSCONFIG_SET_STRING, "source", tst_device->dev, 0));
> > +	if (TST_RET < 0) {
> > +		tst_brk(TFAIL | TTERRNO,
> > +		        "fsconfig set source to %s", tst_device->dev);
> > +	} else {
> 
> Here as well.
> 
> > +		tst_res(TPASS,
> > +			"fsconfig set source to %s", tst_device->dev);
> > +	}
> > +
> > +	TEST(fsconfig(sfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0));
> > +	if (TST_RET < 0) {
> > +		tst_brk(TFAIL | TTERRNO,
> > +		        "fsconfig create superblock");
> 
> And here.
> 
> > +	} else {
> > +		tst_res(TPASS,
> > +			"fsconfig create superblock");
> > +	}
> > +
> > +	TEST(fsmount(sfd, FSMOUNT_CLOEXEC, 0));
> > +	if (TST_RET < 0) {
> > +		tst_brk(TFAIL | TTERRNO, "fsmount");
> > +	} else {
> 
> And here.
> 
> > +		mfd = TST_RET;
> > +		tst_res(TPASS, "fsmount");
> > +		SAFE_CLOSE(sfd);
> > +	}
> > +
> > +	TEST(move_mount(mfd, "", AT_FDCWD, MNTPOINT, MOVE_MOUNT_F_EMPTY_PATH));
> > +	if (TST_RET < 0) {
> > +		tst_brk(TFAIL | TTERRNO, "move_mount attach to mount point");
> > +	} else {
> 
> And here.
> 
> > +		tst_res(TPASS, "move_mount attach to mount point");
> > +		mount_flag = 1;
> > +		if (ismount(MNTPOINT))
> > +			tst_res(TPASS, "new mount works");
> > +		else
> > +			tst_res(TFAIL, "new mount fails");
> > +	}
> > +	SAFE_CLOSE(mfd);
> 
> We have to umount the device here, otherwise it would be mounted for
> each test iteration with -i.

OK, should I keep the 'umount' operation in cleanup() too?

Thanks,
Zorro

> 
> > +}
> > +
> > +struct test_cases {
> > +	void (*tfunc)(void);
> > +} tcases[] = {
> > +	{&test_newmount},
> > +};
> 
> Unless you plan to add more tests here, there is no point in declaring
> the structure with function pointers.
> 
> > +static void run(unsigned int i)
> > +{
> > +	tcases[i].tfunc();
> > +}
> > +
> > +static struct tst_test test = {
> > +	.test		= run,
> > +	.tcnt		= ARRAY_SIZE(tcases),
> > +	.setup		= setup,
> > +	.cleanup	= cleanup,
> > +	.needs_root	= 1,
> > +	.mntpoint	= MNTPOINT,
> > +	.needs_device	= 1,
> > +};
> 
> Otherwise it looks good.
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 17:35 [PATCH] syscalls/newmount: new test case for new mount API Zorro Lang
2019-11-28 19:14 ` Petr Vorel
2019-11-29  3:48   ` Zorro Lang
     [not found] ` <9c487d75-0de0-af8f-a439-d3ce9d965808@cn.fujitsu.com>
2019-11-29  5:29   ` [LTP] " Yang Xu
2019-11-29 11:20     ` Zorro Lang
2019-12-03 13:03 ` Cyril Hrubis
2019-12-06 16:23   ` Zorro Lang
2019-12-06 16:18     ` Cyril Hrubis

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git