[Cluster-devel] [PATCH 1/3] libgfs2: Add lgfs2_open_mnt* functions

[Cluster-devel] [PATCH 1/3] libgfs2: Add lgfs2_open_mnt* functions

[Andrew Price]

lgfs2_open_mnt is a replacement for is_pathname_mounted which tries to reduce
races by opening paths speculatively and passing back the open fds once they're
known to be correct. lgfs2_open_mnt_{dev,dir} build on this to provide a
convenient way to open just the device or mount directory relating to a path
which could be either.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/libgfs2/libgfs2.h |  4 +++
 gfs2/libgfs2/misc.c    | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index f864a08..3e5d09c 100644
--- a/gfs2/libgfs2/libgfs2.h
+++ b/gfs2/libgfs2/libgfs2.h
@@ -12,6 +12,7 @@
 #include <linux/limits.h>
 #include <endian.h>
 #include <byteswap.h>
+#include <mntent.h>
 
 #include <linux/gfs2_ondisk.h>
 #include "osi_list.h"
@@ -715,6 +716,9 @@ extern int compute_heightsize(struct gfs2_sbd *sdp, uint64_t *heightsize,
 		uint32_t *maxheight, uint32_t bsize1, int diptrs, int inptrs);
 extern int compute_constants(struct gfs2_sbd *sdp);
 extern int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount);
+extern int lgfs2_open_mnt(const char *path, int dirflags, int *dirfd, int devflags, int *devfd, struct mntent **mnt);
+extern int lgfs2_open_mnt_dev(const char *path, int flags, struct mntent **mnt);
+extern int lgfs2_open_mnt_dir(const char *path, int flags, struct mntent **mnt);
 extern int find_gfs2_meta(struct gfs2_sbd *sdp);
 extern int dir_exists(const char *dir);
 extern int mount_gfs2_meta(struct gfs2_sbd *sdp);
diff --git a/gfs2/libgfs2/misc.c b/gfs2/libgfs2/misc.c
index 7f500e6..195b983 100644
--- a/gfs2/libgfs2/misc.c
+++ b/gfs2/libgfs2/misc.c
@@ -163,6 +163,100 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
 	return 1; /* mounted */
 }
 
+/* Returns 0 if fd1 and fd2 refer to the same device/file, 1 otherwise, or -1 on error */
+static int fdcmp(int fd1, int fd2)
+{
+	struct stat st1, st2;
+	if ((fstat(fd1, &st1) != 0) || (fstat(fd2, &st2) != 0))
+		return -1;
+	if (S_ISBLK(st1.st_mode) && S_ISBLK(st2.st_mode)) {
+		if (st1.st_rdev == st2.st_rdev) {
+			return 0;
+		}
+	} else if ((st1.st_dev == st2.st_dev) && (st1.st_ino == st2.st_ino)) {
+			return 0;
+	}
+	return 1;
+}
+
+int lgfs2_open_mnt(const char *path, int dirflags, int *dirfd, int devflags, int *devfd, struct mntent **mnt)
+{
+	FILE *fp = setmntent("/proc/mounts", "r");
+	if (fp == NULL) {
+		perror("open: /proc/mounts");
+		return 1;
+	}
+	/* Assume path is mount point until we know better. */
+	*dirfd = open(path, dirflags);
+	if (*dirfd < 0)
+		return 1;
+
+	while ((*mnt = getmntent(fp)) != NULL) {
+		int fd;
+		if (strcmp((*mnt)->mnt_type, "gfs2") != 0)
+			continue;
+		*devfd = open((*mnt)->mnt_fsname, devflags);
+		/* Defer checking *devfd until later: whether it's ok to ignore
+		 * the error depends on whether we find the mount point. */
+
+		if (strcmp(path, (*mnt)->mnt_dir) == 0)
+			break;
+		if (strcmp(path, (*mnt)->mnt_fsname) == 0 || fdcmp(*dirfd, *devfd) == 0) {
+			/* We have a match but our above assumption was
+			   incorrect and *dirfd is actually the device. */
+			close(*dirfd);
+			*dirfd = open((*mnt)->mnt_dir, dirflags);
+			break;
+		}
+
+		fd = open((*mnt)->mnt_dir, dirflags);
+		if (fd >= 0) {
+			int diff = fdcmp(*dirfd, fd);
+			close(fd);
+			if (diff == 0)
+				break;
+		}
+		if (*devfd >= 0)
+			close(*devfd);
+	}
+	endmntent(fp);
+	if (*mnt == NULL) {
+		close(*dirfd);
+		return 0; /* Success. Answer is no. Both fds closed. */
+	}
+	if (*dirfd < 0) {
+		close(*devfd);
+		return 1;
+	}
+	if (*devfd < 0) {
+		close(*dirfd);
+		return 1;
+	}
+	return 0; /* Success. Answer is yes. Both fds open. */
+}
+
+int lgfs2_open_mnt_dev(const char *path, int flags, struct mntent **mnt)
+{
+	int dirfd = -1;
+	int devfd = -1;
+	if (lgfs2_open_mnt(path, O_RDONLY, &dirfd, flags, &devfd, mnt) != 0)
+		return -1;
+	if (*mnt != NULL)
+		close(dirfd);
+	return devfd;
+}
+
+int lgfs2_open_mnt_dir(const char *path, int flags, struct mntent **mnt)
+{
+	int dirfd = -1;
+	int devfd = -1;
+	if (lgfs2_open_mnt(path, flags, &dirfd, O_RDONLY, &devfd, mnt) != 0)
+		return -1;
+	if (*mnt != NULL)
+		close(devfd);
+	return dirfd;
+}
+
 static int lock_for_admin(struct gfs2_sbd *sdp)
 {
 	int error;
-- 
1.8.3.1

[Cluster-devel] [PATCH 2/3] Switch is_pathname_mounted callers to lgfs2_open_mnt*

[Andrew Price]

Use the new lgfs2_open_mnt* functions in fsck.gfs2, gfs2_grow, and gfs2_jadd.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c | 30 ++++++++++++------------------
 gfs2/mkfs/main_grow.c  | 31 ++++++++++++-------------------
 gfs2/mkfs/main_jadd.c  | 17 +++++++++--------
 3 files changed, 33 insertions(+), 45 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index 6612fe7..5758607 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -1475,8 +1475,7 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
 
 	sdp->device_fd = open(opts.device, open_flag);
 	if (sdp->device_fd < 0) {
-		int is_mounted, ro;
-
+		struct mntent *mnt;
 		if (open_flag == O_RDONLY || errno != EBUSY) {
 			log_crit( _("Unable to open device: %s\n"),
 				  opts.device);
@@ -1485,30 +1484,23 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
 		/* We can't open it EXCL.  It may be already open rw (in which
 		   case we want to deny them access) or it may be mounted as
 		   the root file system@boot time (in which case we need to
-		   allow it.)  We use is_pathname_mounted here even though
-		   we're specifying a device name, not a path name.  The
-		   function checks for device as well. */
-		strncpy(sdp->device_name, opts.device,
-			sizeof(sdp->device_name));
-		sdp->path_name = sdp->device_name; /* This gets overwritten */
-		is_mounted = is_pathname_mounted(sdp->path_name, sdp->device_name, &ro);
-		/* If the device is busy, but not because it's mounted, fail.
+		   allow it.)
+		   If the device is busy, but not because it's mounted, fail.
 		   This protects against cases where the file system is LVM
-		   and perhaps mounted on a different node. */
-		if (!is_mounted)
+		   and perhaps mounted on a different node.
+		   Try opening without O_EXCL. */
+		sdp->device_fd = lgfs2_open_mnt_dev(opts.device, O_RDWR, &mnt);
+		if (sdp->device_fd < 0)
 			goto mount_fail;
 		/* If the device is mounted, but not mounted RO, fail.  This
 		   protects them against cases where the file system is
 		   mounted RW, but still allows us to check our own root
 		   file system. */
-		if (!ro)
-			goto mount_fail;
+		if (!hasmntopt(mnt, MNTOPT_RO))
+			goto close_fail;
 		/* The device is mounted RO, so it's likely our own root
 		   file system.  We can only do so much to protect the users
-		   from themselves.  Try opening without O_EXCL. */
-		if ((sdp->device_fd = open(opts.device, O_RDWR)) < 0)
-			goto mount_fail;
-
+		   from themselves. */
 		was_mounted_ro = 1;
 	}
 
@@ -1591,6 +1583,8 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
 
 	return FSCK_OK;
 
+close_fail:
+	close(sdp->device_fd);
 mount_fail:
 	log_crit( _("Device %s is busy.\n"), opts.device);
 	return FSCK_USAGE;
diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
index 5db91d9..541b0f2 100644
--- a/gfs2/mkfs/main_grow.c
+++ b/gfs2/mkfs/main_grow.c
@@ -323,7 +323,7 @@ main_grow(int argc, char *argv[])
 	int rgcount, rindex_fd;
 	char rindex_name[PATH_MAX];
 	int error = EXIT_SUCCESS;
-	int ro_mnt = 0;
+	int devflags = (test ? O_RDONLY : O_RDWR) | O_CLOEXEC;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->bsize = GFS2_DEFAULT_BSIZE;
@@ -333,30 +333,23 @@ main_grow(int argc, char *argv[])
 	sdp->md.journals = 1;
 	decode_arguments(argc, argv, sdp);
 	
-	while ((argc - optind) > 0) {
+	for(; (argc - optind) > 0; optind++) {
 		int sane;
+		struct mntent *mnt;
 		struct rgrp_tree *last_rgrp;
 
-		strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
-		sdp->path_name = argv[optind++];
-
-		if ((!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt))) {
-			perror(sdp->path_name);
-			exit(EXIT_FAILURE);
-		}
-
-		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
-		if (sdp->path_fd < 0){
-			perror(sdp->path_name);
+		error = lgfs2_open_mnt(argv[optind], O_RDONLY|O_CLOEXEC, &sdp->path_fd,
+		                                       devflags, &sdp->device_fd, &mnt);
+		if (error != 0) {
+			fprintf(stderr, _("Error looking up mount '%s': %s\n"), argv[optind], strerror(errno));
 			exit(EXIT_FAILURE);
 		}
-
-		sdp->device_fd = open(sdp->device_name,
-				      (test ? O_RDONLY : O_RDWR) | O_CLOEXEC);
-		if (sdp->device_fd < 0){
-			perror(sdp->device_name);
-			exit(EXIT_FAILURE);
+		if (mnt == NULL) {
+			fprintf(stderr, _("%s: not a mounted gfs2 file system\n"), argv[optind]);
+			continue;
 		}
+		sdp->path_name = mnt->mnt_dir;
+		strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
 
 		if (lgfs2_get_dev_info(sdp->device_fd, &sdp->dinfo) < 0) {
 			perror(sdp->device_name);
diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
index b6cd8e4..815dd52 100644
--- a/gfs2/mkfs/main_jadd.c
+++ b/gfs2/mkfs/main_jadd.c
@@ -490,8 +490,8 @@ add_j(struct gfs2_sbd *sdp)
 void main_jadd(int argc, char *argv[])
 {
 	struct gfs2_sbd sbd, *sdp = &sbd;
+	struct mntent *mnt;
 	unsigned int total;
-	int ro_mnt = 0;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->jsize = GFS2_DEFAULT_JSIZE;
@@ -500,17 +500,18 @@ void main_jadd(int argc, char *argv[])
 
 	decode_arguments(argc, argv, sdp);
 	verify_arguments(sdp);
-	
-	sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
-	if (sdp->path_fd < 0){
-		perror(sdp->path_name);
+
+	sbd.path_fd = lgfs2_open_mnt_dir(sbd.path_name, O_RDONLY|O_CLOEXEC, &mnt);
+	if (sbd.path_fd < 0) {
+		fprintf(stderr, _("Error looking up mount '%s': %s\n"), sbd.path_name, strerror(errno));
 		exit(EXIT_FAILURE);
 	}
-
-	if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
-		perror(sdp->path_name);
+	if (mnt == NULL) {
+		fprintf(stderr, _("%s: not a mounted gfs2 file system\n"), sbd.path_name);
 		exit(EXIT_FAILURE);
 	}
+	sbd.path_name = mnt->mnt_dir;
+	strncpy(sbd.device_name, mnt->mnt_fsname, PATH_MAX - 1);
 
 	gather_info(sdp);
 
-- 
1.8.3.1

[Cluster-devel] [PATCH 3/3] libgfs2: Remove is_pathname_mounted

[Andrew Price]

All callers are now using lgfs2_open_mnt* instead.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/libgfs2/libgfs2.h |  1 -
 gfs2/libgfs2/misc.c    | 61 --------------------------------------------------
 2 files changed, 62 deletions(-)

diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index 3e5d09c..e785017 100644
--- a/gfs2/libgfs2/libgfs2.h
+++ b/gfs2/libgfs2/libgfs2.h
@@ -715,7 +715,6 @@ extern int metafs_interrupted;
 extern int compute_heightsize(struct gfs2_sbd *sdp, uint64_t *heightsize,
 		uint32_t *maxheight, uint32_t bsize1, int diptrs, int inptrs);
 extern int compute_constants(struct gfs2_sbd *sdp);
-extern int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount);
 extern int lgfs2_open_mnt(const char *path, int dirflags, int *dirfd, int devflags, int *devfd, struct mntent **mnt);
 extern int lgfs2_open_mnt_dev(const char *path, int flags, struct mntent **mnt);
 extern int lgfs2_open_mnt_dir(const char *path, int flags, struct mntent **mnt);
diff --git a/gfs2/libgfs2/misc.c b/gfs2/libgfs2/misc.c
index 195b983..c4ed722 100644
--- a/gfs2/libgfs2/misc.c
+++ b/gfs2/libgfs2/misc.c
@@ -102,67 +102,6 @@ int compute_constants(struct gfs2_sbd *sdp)
 	return 0;
 }
 
-int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
-{
-	FILE *fp;
-	struct mntent *mnt;
-	dev_t file_dev=0, file_rdev=0;
-	ino_t file_ino=0;
-	struct stat st_buf;
-
-	*ro_mount = 0;
-	if ((fp = setmntent("/proc/mounts", "r")) == NULL) {
-		perror("open: /proc/mounts");
-		return 0;
-	}
-	if (stat(path_name, &st_buf) == 0) {
-		if (S_ISBLK(st_buf.st_mode)) {
-#ifndef __GNU__ /* The GNU hurd is broken with respect to stat devices */
-			file_rdev = st_buf.st_rdev;
-#endif  /* __GNU__ */
-		} else {
-			file_dev = st_buf.st_dev;
-			file_ino = st_buf.st_ino;
-		}
-	}
-	while ((mnt = getmntent (fp)) != NULL) {
-		/* Check if they specified the device instead of mnt point */
-		if (strcmp(device_name, mnt->mnt_fsname) == 0) {
-			strcpy(path_name, mnt->mnt_dir); /* fix it */
-			break;
-		}
-		if (strcmp(path_name, mnt->mnt_dir) == 0) {
-			strcpy(device_name, mnt->mnt_fsname); /* fix it */
-			break;
-		}
-		if (stat(mnt->mnt_fsname, &st_buf) == 0) {
-			if (S_ISBLK(st_buf.st_mode)) {
-#ifndef __GNU__
-				if (file_rdev && (file_rdev == st_buf.st_rdev))
-					break;
-#endif  /* __GNU__ */
-			} else {
-				if (file_dev && ((file_dev == st_buf.st_dev) &&
-						 (file_ino == st_buf.st_ino)))
-					break;
-			}
-		}
-	}
-	endmntent (fp);
-	if (mnt == NULL)
-		return 0;
-	if (stat(mnt->mnt_dir, &st_buf) < 0) {
-		if (errno == ENOENT)
-			return 0;
-	}
-	/* Can't trust fstype because / has "rootfs". */
-	if (file_rdev && (st_buf.st_dev != file_rdev))
-		return 0;
-	if (hasmntopt(mnt, MNTOPT_RO))
-               *ro_mount = 1;
-	return 1; /* mounted */
-}
-
 /* Returns 0 if fd1 and fd2 refer to the same device/file, 1 otherwise, or -1 on error */
 static int fdcmp(int fd1, int fd2)
 {
-- 
1.8.3.1

[Cluster-devel] [PATCH 1/3] libgfs2: Add lgfs2_open_mnt* functions

[Steven Whitehouse]

Hi,

That looks much better I than what we had before I think. Thanks for
sorting that out,

Steve.

On Sat, 2013-11-16 at 20:11 +0000, Andrew Price wrote:
> lgfs2_open_mnt is a replacement for is_pathname_mounted which tries to reduce
> races by opening paths speculatively and passing back the open fds once they're
> known to be correct. lgfs2_open_mnt_{dev,dir} build on this to provide a
> convenient way to open just the device or mount directory relating to a path
> which could be either.
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  gfs2/libgfs2/libgfs2.h |  4 +++
>  gfs2/libgfs2/misc.c    | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
> index f864a08..3e5d09c 100644
> --- a/gfs2/libgfs2/libgfs2.h
> +++ b/gfs2/libgfs2/libgfs2.h
> @@ -12,6 +12,7 @@
>  #include <linux/limits.h>
>  #include <endian.h>
>  #include <byteswap.h>
> +#include <mntent.h>
>  
>  #include <linux/gfs2_ondisk.h>
>  #include "osi_list.h"
> @@ -715,6 +716,9 @@ extern int compute_heightsize(struct gfs2_sbd *sdp, uint64_t *heightsize,
>  		uint32_t *maxheight, uint32_t bsize1, int diptrs, int inptrs);
>  extern int compute_constants(struct gfs2_sbd *sdp);
>  extern int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount);
> +extern int lgfs2_open_mnt(const char *path, int dirflags, int *dirfd, int devflags, int *devfd, struct mntent **mnt);
> +extern int lgfs2_open_mnt_dev(const char *path, int flags, struct mntent **mnt);
> +extern int lgfs2_open_mnt_dir(const char *path, int flags, struct mntent **mnt);
>  extern int find_gfs2_meta(struct gfs2_sbd *sdp);
>  extern int dir_exists(const char *dir);
>  extern int mount_gfs2_meta(struct gfs2_sbd *sdp);
> diff --git a/gfs2/libgfs2/misc.c b/gfs2/libgfs2/misc.c
> index 7f500e6..195b983 100644
> --- a/gfs2/libgfs2/misc.c
> +++ b/gfs2/libgfs2/misc.c
> @@ -163,6 +163,100 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
>  	return 1; /* mounted */
>  }
>  
> +/* Returns 0 if fd1 and fd2 refer to the same device/file, 1 otherwise, or -1 on error */
> +static int fdcmp(int fd1, int fd2)
> +{
> +	struct stat st1, st2;
> +	if ((fstat(fd1, &st1) != 0) || (fstat(fd2, &st2) != 0))
> +		return -1;
> +	if (S_ISBLK(st1.st_mode) && S_ISBLK(st2.st_mode)) {
> +		if (st1.st_rdev == st2.st_rdev) {
> +			return 0;
> +		}
> +	} else if ((st1.st_dev == st2.st_dev) && (st1.st_ino == st2.st_ino)) {
> +			return 0;
> +	}
> +	return 1;
> +}
> +
> +int lgfs2_open_mnt(const char *path, int dirflags, int *dirfd, int devflags, int *devfd, struct mntent **mnt)
> +{
> +	FILE *fp = setmntent("/proc/mounts", "r");
> +	if (fp == NULL) {
> +		perror("open: /proc/mounts");
> +		return 1;
> +	}
> +	/* Assume path is mount point until we know better. */
> +	*dirfd = open(path, dirflags);
> +	if (*dirfd < 0)
> +		return 1;
> +
> +	while ((*mnt = getmntent(fp)) != NULL) {
> +		int fd;
> +		if (strcmp((*mnt)->mnt_type, "gfs2") != 0)
> +			continue;
> +		*devfd = open((*mnt)->mnt_fsname, devflags);
> +		/* Defer checking *devfd until later: whether it's ok to ignore
> +		 * the error depends on whether we find the mount point. */
> +
> +		if (strcmp(path, (*mnt)->mnt_dir) == 0)
> +			break;
> +		if (strcmp(path, (*mnt)->mnt_fsname) == 0 || fdcmp(*dirfd, *devfd) == 0) {
> +			/* We have a match but our above assumption was
> +			   incorrect and *dirfd is actually the device. */
> +			close(*dirfd);
> +			*dirfd = open((*mnt)->mnt_dir, dirflags);
> +			break;
> +		}
> +
> +		fd = open((*mnt)->mnt_dir, dirflags);
> +		if (fd >= 0) {
> +			int diff = fdcmp(*dirfd, fd);
> +			close(fd);
> +			if (diff == 0)
> +				break;
> +		}
> +		if (*devfd >= 0)
> +			close(*devfd);
> +	}
> +	endmntent(fp);
> +	if (*mnt == NULL) {
> +		close(*dirfd);
> +		return 0; /* Success. Answer is no. Both fds closed. */
> +	}
> +	if (*dirfd < 0) {
> +		close(*devfd);
> +		return 1;
> +	}
> +	if (*devfd < 0) {
> +		close(*dirfd);
> +		return 1;
> +	}
> +	return 0; /* Success. Answer is yes. Both fds open. */
> +}
> +
> +int lgfs2_open_mnt_dev(const char *path, int flags, struct mntent **mnt)
> +{
> +	int dirfd = -1;
> +	int devfd = -1;
> +	if (lgfs2_open_mnt(path, O_RDONLY, &dirfd, flags, &devfd, mnt) != 0)
> +		return -1;
> +	if (*mnt != NULL)
> +		close(dirfd);
> +	return devfd;
> +}
> +
> +int lgfs2_open_mnt_dir(const char *path, int flags, struct mntent **mnt)
> +{
> +	int dirfd = -1;
> +	int devfd = -1;
> +	if (lgfs2_open_mnt(path, flags, &dirfd, O_RDONLY, &devfd, mnt) != 0)
> +		return -1;
> +	if (*mnt != NULL)
> +		close(devfd);
> +	return dirfd;
> +}
> +
>  static int lock_for_admin(struct gfs2_sbd *sdp)
>  {
>  	int error;