[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it

[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it

[Andrew Price]

is_pathname_mounted is renamed lgfs2_find_mnt and now returns more
context through the isdev and mnt parameters. It now only takes one path
and returns whether it is the name of the device or the mount point
using *isdev. It also sets *mnt to the struct mntent found by
getmntent() so that the caller can check its contents itself.

Callers have been updated and a bug where gfs2_grow wasn't working when
the device name was provided by the user has been fixed.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/fsck/initialize.c |  9 ++++-----
 gfs2/libgfs2/libgfs2.h |  3 ++-
 gfs2/libgfs2/misc.c    | 55 ++++++++++++++++++++++++++++++--------------------
 gfs2/mkfs/main_grow.c  | 24 ++++++++++++++++------
 gfs2/mkfs/main_jadd.c  | 23 ++++++++++++++++-----
 5 files changed, 75 insertions(+), 39 deletions(-)

diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
index 6612fe7..1cc15ec 100644
--- a/gfs2/fsck/initialize.c
+++ b/gfs2/fsck/initialize.c
@@ -1475,7 +1475,8 @@ 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;
+		int isdev;
+		struct mntent *mnt;
 
 		if (open_flag == O_RDONLY || errno != EBUSY) {
 			log_crit( _("Unable to open device: %s\n"),
@@ -1490,18 +1491,16 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
 		   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.
 		   This protects against cases where the file system is LVM
 		   and perhaps mounted on a different node. */
-		if (!is_mounted)
+		if (lgfs2_find_mnt(opts.device, &isdev, &mnt) || mnt == NULL)
 			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)
+		if (!hasmntopt(mnt, MNTOPT_RO))
 			goto mount_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
diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index f864a08..993f605 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"
@@ -714,7 +715,7 @@ 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_find_mnt(const char *path, int *isdev, 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..6bc95a9 100644
--- a/gfs2/libgfs2/misc.c
+++ b/gfs2/libgfs2/misc.c
@@ -102,20 +102,27 @@ int compute_constants(struct gfs2_sbd *sdp)
 	return 0;
 }
 
-int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
+/**
+ * lgfs2_find_mnt - Find a mounted file system relating to a path.
+ * path:  The path
+ * isdev: Will be non-zero if the path is the device or zero if the path is the mount directory
+ * mnt:   When the return value is 0, this will either be NULL for "not found" or
+ *        it will point to the found mount point.
+ * Returns 0 to indicate a successful scan of the mount points or non-zero on error with errno set.
+ *         On success, *mnt will be NULL or a valid pointer as noted above.
+ */
+int lgfs2_find_mnt(const char *path, int *isdev, struct mntent **mnt)
 {
 	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;
+		return 1;
 	}
-	if (stat(path_name, &st_buf) == 0) {
+	if (stat(path, &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;
@@ -125,42 +132,46 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
 			file_ino = st_buf.st_ino;
 		}
 	}
-	while ((mnt = getmntent (fp)) != NULL) {
+	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 */
+		if (strcmp(path, (*mnt)->mnt_fsname) == 0) {
+			*isdev = 1;
 			break;
 		}
-		if (strcmp(path_name, mnt->mnt_dir) == 0) {
-			strcpy(device_name, mnt->mnt_fsname); /* fix it */
+		if (strcmp(path, (*mnt)->mnt_dir) == 0) {
+			*isdev = 0;
 			break;
 		}
-		if (stat(mnt->mnt_fsname, &st_buf) == 0) {
+		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))
+				if (file_rdev && (file_rdev == st_buf.st_rdev)) {
+					*isdev = 1;
 					break;
+				}
 #endif  /* __GNU__ */
 			} else {
 				if (file_dev && ((file_dev == st_buf.st_dev) &&
-						 (file_ino == st_buf.st_ino)))
+						 (file_ino == st_buf.st_ino))) {
+					*isdev = 0;
 					break;
+				}
 			}
 		}
 	}
 	endmntent (fp);
-	if (mnt == NULL)
-		return 0;
-	if (stat(mnt->mnt_dir, &st_buf) < 0) {
+	if (*mnt == NULL)
+		return 0; /* Success. Answer is no. */
+	if (stat((*mnt)->mnt_dir, &st_buf) < 0) {
 		if (errno == ENOENT)
-			return 0;
+			return 1;
 	}
 	/* 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 */
+	if (file_rdev && (st_buf.st_dev != file_rdev)) {
+		errno = EINVAL;
+		return 1;
+	}
+	return 0; /* Success. Answer is yes. */
 }
 
 static int lock_for_admin(struct gfs2_sbd *sdp)
diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
index 5db91d9..4c8807e 100644
--- a/gfs2/mkfs/main_grow.c
+++ b/gfs2/mkfs/main_grow.c
@@ -323,7 +323,6 @@ main_grow(int argc, char *argv[])
 	int rgcount, rindex_fd;
 	char rindex_name[PATH_MAX];
 	int error = EXIT_SUCCESS;
-	int ro_mnt = 0;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->bsize = GFS2_DEFAULT_BSIZE;
@@ -335,16 +334,29 @@ main_grow(int argc, char *argv[])
 	
 	while ((argc - optind) > 0) {
 		int sane;
+		int isdev = 0;
+		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);
+		if (lgfs2_find_mnt(argv[optind], &isdev, &mnt)) {
+			perror(argv[optind]);
 			exit(EXIT_FAILURE);
 		}
 
+		if (mnt == NULL) {
+			fprintf(stderr, "%s: not a mounted gfs2 file system\n", argv[optind++]);
+			continue;
+		}
+
+		if (isdev) {
+			strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
+			sdp->path_name = mnt->mnt_dir;
+		} else {
+			strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
+			sdp->path_name = argv[optind];
+		}
+		optind++;
+
 		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
 		if (sdp->path_fd < 0){
 			perror(sdp->path_name);
diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
index b6cd8e4..9240838 100644
--- a/gfs2/mkfs/main_jadd.c
+++ b/gfs2/mkfs/main_jadd.c
@@ -490,8 +490,9 @@ 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;
+	int isdev;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->jsize = GFS2_DEFAULT_JSIZE;
@@ -500,14 +501,26 @@ 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){
+
+	if (lgfs2_find_mnt(sdp->path_name, &isdev, &mnt)) {
 		perror(sdp->path_name);
 		exit(EXIT_FAILURE);
 	}
 
-	if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
+	if (mnt == NULL) {
+		fprintf(stderr, "%s: not a mounted gfs2 file system\n", sdp->path_name);
+		exit(EXIT_FAILURE);
+	}
+
+	if (isdev) {
+		strncpy(sdp->device_name, sdp->path_name, PATH_MAX - 1);
+		sdp->path_name = mnt->mnt_dir;
+	} else {
+		strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
+	}
+
+	sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
+	if (sdp->path_fd < 0){
 		perror(sdp->path_name);
 		exit(EXIT_FAILURE);
 	}
-- 
1.8.3.1

[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it

[Andrew Price]

This one fell off my radar somehow. Reviews appreciated.

Andy

On 12/10/13 23:14, Andrew Price wrote:
> is_pathname_mounted is renamed lgfs2_find_mnt and now returns more
> context through the isdev and mnt parameters. It now only takes one path
> and returns whether it is the name of the device or the mount point
> using *isdev. It also sets *mnt to the struct mntent found by
> getmntent() so that the caller can check its contents itself.
>
> Callers have been updated and a bug where gfs2_grow wasn't working when
> the device name was provided by the user has been fixed.
>
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>   gfs2/fsck/initialize.c |  9 ++++-----
>   gfs2/libgfs2/libgfs2.h |  3 ++-
>   gfs2/libgfs2/misc.c    | 55 ++++++++++++++++++++++++++++++--------------------
>   gfs2/mkfs/main_grow.c  | 24 ++++++++++++++++------
>   gfs2/mkfs/main_jadd.c  | 23 ++++++++++++++++-----
>   5 files changed, 75 insertions(+), 39 deletions(-)
>
> diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
> index 6612fe7..1cc15ec 100644
> --- a/gfs2/fsck/initialize.c
> +++ b/gfs2/fsck/initialize.c
> @@ -1475,7 +1475,8 @@ 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;
> +		int isdev;
> +		struct mntent *mnt;
>
>   		if (open_flag == O_RDONLY || errno != EBUSY) {
>   			log_crit( _("Unable to open device: %s\n"),
> @@ -1490,18 +1491,16 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
>   		   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.
>   		   This protects against cases where the file system is LVM
>   		   and perhaps mounted on a different node. */
> -		if (!is_mounted)
> +		if (lgfs2_find_mnt(opts.device, &isdev, &mnt) || mnt == NULL)
>   			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)
> +		if (!hasmntopt(mnt, MNTOPT_RO))
>   			goto mount_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
> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
> index f864a08..993f605 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"
> @@ -714,7 +715,7 @@ 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_find_mnt(const char *path, int *isdev, 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..6bc95a9 100644
> --- a/gfs2/libgfs2/misc.c
> +++ b/gfs2/libgfs2/misc.c
> @@ -102,20 +102,27 @@ int compute_constants(struct gfs2_sbd *sdp)
>   	return 0;
>   }
>
> -int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
> +/**
> + * lgfs2_find_mnt - Find a mounted file system relating to a path.
> + * path:  The path
> + * isdev: Will be non-zero if the path is the device or zero if the path is the mount directory
> + * mnt:   When the return value is 0, this will either be NULL for "not found" or
> + *        it will point to the found mount point.
> + * Returns 0 to indicate a successful scan of the mount points or non-zero on error with errno set.
> + *         On success, *mnt will be NULL or a valid pointer as noted above.
> + */
> +int lgfs2_find_mnt(const char *path, int *isdev, struct mntent **mnt)
>   {
>   	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;
> +		return 1;
>   	}
> -	if (stat(path_name, &st_buf) == 0) {
> +	if (stat(path, &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;
> @@ -125,42 +132,46 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
>   			file_ino = st_buf.st_ino;
>   		}
>   	}
> -	while ((mnt = getmntent (fp)) != NULL) {
> +	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 */
> +		if (strcmp(path, (*mnt)->mnt_fsname) == 0) {
> +			*isdev = 1;
>   			break;
>   		}
> -		if (strcmp(path_name, mnt->mnt_dir) == 0) {
> -			strcpy(device_name, mnt->mnt_fsname); /* fix it */
> +		if (strcmp(path, (*mnt)->mnt_dir) == 0) {
> +			*isdev = 0;
>   			break;
>   		}
> -		if (stat(mnt->mnt_fsname, &st_buf) == 0) {
> +		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))
> +				if (file_rdev && (file_rdev == st_buf.st_rdev)) {
> +					*isdev = 1;
>   					break;
> +				}
>   #endif  /* __GNU__ */
>   			} else {
>   				if (file_dev && ((file_dev == st_buf.st_dev) &&
> -						 (file_ino == st_buf.st_ino)))
> +						 (file_ino == st_buf.st_ino))) {
> +					*isdev = 0;
>   					break;
> +				}
>   			}
>   		}
>   	}
>   	endmntent (fp);
> -	if (mnt == NULL)
> -		return 0;
> -	if (stat(mnt->mnt_dir, &st_buf) < 0) {
> +	if (*mnt == NULL)
> +		return 0; /* Success. Answer is no. */
> +	if (stat((*mnt)->mnt_dir, &st_buf) < 0) {
>   		if (errno == ENOENT)
> -			return 0;
> +			return 1;
>   	}
>   	/* 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 */
> +	if (file_rdev && (st_buf.st_dev != file_rdev)) {
> +		errno = EINVAL;
> +		return 1;
> +	}
> +	return 0; /* Success. Answer is yes. */
>   }
>
>   static int lock_for_admin(struct gfs2_sbd *sdp)
> diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
> index 5db91d9..4c8807e 100644
> --- a/gfs2/mkfs/main_grow.c
> +++ b/gfs2/mkfs/main_grow.c
> @@ -323,7 +323,6 @@ main_grow(int argc, char *argv[])
>   	int rgcount, rindex_fd;
>   	char rindex_name[PATH_MAX];
>   	int error = EXIT_SUCCESS;
> -	int ro_mnt = 0;
>
>   	memset(sdp, 0, sizeof(struct gfs2_sbd));
>   	sdp->bsize = GFS2_DEFAULT_BSIZE;
> @@ -335,16 +334,29 @@ main_grow(int argc, char *argv[])
>   	
>   	while ((argc - optind) > 0) {
>   		int sane;
> +		int isdev = 0;
> +		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);
> +		if (lgfs2_find_mnt(argv[optind], &isdev, &mnt)) {
> +			perror(argv[optind]);
>   			exit(EXIT_FAILURE);
>   		}
>
> +		if (mnt == NULL) {
> +			fprintf(stderr, "%s: not a mounted gfs2 file system\n", argv[optind++]);
> +			continue;
> +		}
> +
> +		if (isdev) {
> +			strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
> +			sdp->path_name = mnt->mnt_dir;
> +		} else {
> +			strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
> +			sdp->path_name = argv[optind];
> +		}
> +		optind++;
> +
>   		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
>   		if (sdp->path_fd < 0){
>   			perror(sdp->path_name);
> diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
> index b6cd8e4..9240838 100644
> --- a/gfs2/mkfs/main_jadd.c
> +++ b/gfs2/mkfs/main_jadd.c
> @@ -490,8 +490,9 @@ 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;
> +	int isdev;
>
>   	memset(sdp, 0, sizeof(struct gfs2_sbd));
>   	sdp->jsize = GFS2_DEFAULT_JSIZE;
> @@ -500,14 +501,26 @@ 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){
> +
> +	if (lgfs2_find_mnt(sdp->path_name, &isdev, &mnt)) {
>   		perror(sdp->path_name);
>   		exit(EXIT_FAILURE);
>   	}
>
> -	if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
> +	if (mnt == NULL) {
> +		fprintf(stderr, "%s: not a mounted gfs2 file system\n", sdp->path_name);
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	if (isdev) {
> +		strncpy(sdp->device_name, sdp->path_name, PATH_MAX - 1);
> +		sdp->path_name = mnt->mnt_dir;
> +	} else {
> +		strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
> +	}
> +
> +	sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
> +	if (sdp->path_fd < 0){
>   		perror(sdp->path_name);
>   		exit(EXIT_FAILURE);
>   	}
>

[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it

[Steven Whitehouse]

Hi,

Since the path has already been transformed into an fd, we should be
using the fd rather than sending the path to lgfs2_find_mnt in order to
avoid any possible races and also issues wrt device names being
different,

Steve.

On Mon, 2013-11-04 at 15:38 +0000, Andrew Price wrote:
> This one fell off my radar somehow. Reviews appreciated.
> 
> Andy
> 
> On 12/10/13 23:14, Andrew Price wrote:
> > is_pathname_mounted is renamed lgfs2_find_mnt and now returns more
> > context through the isdev and mnt parameters. It now only takes one path
> > and returns whether it is the name of the device or the mount point
> > using *isdev. It also sets *mnt to the struct mntent found by
> > getmntent() so that the caller can check its contents itself.
> >
> > Callers have been updated and a bug where gfs2_grow wasn't working when
> > the device name was provided by the user has been fixed.
> >
> > Signed-off-by: Andrew Price <anprice@redhat.com>
> > ---
> >   gfs2/fsck/initialize.c |  9 ++++-----
> >   gfs2/libgfs2/libgfs2.h |  3 ++-
> >   gfs2/libgfs2/misc.c    | 55 ++++++++++++++++++++++++++++++--------------------
> >   gfs2/mkfs/main_grow.c  | 24 ++++++++++++++++------
> >   gfs2/mkfs/main_jadd.c  | 23 ++++++++++++++++-----
> >   5 files changed, 75 insertions(+), 39 deletions(-)
> >
> > diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
> > index 6612fe7..1cc15ec 100644
> > --- a/gfs2/fsck/initialize.c
> > +++ b/gfs2/fsck/initialize.c
> > @@ -1475,7 +1475,8 @@ 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;
> > +		int isdev;
> > +		struct mntent *mnt;
> >
> >   		if (open_flag == O_RDONLY || errno != EBUSY) {
> >   			log_crit( _("Unable to open device: %s\n"),
> > @@ -1490,18 +1491,16 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
> >   		   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.
> >   		   This protects against cases where the file system is LVM
> >   		   and perhaps mounted on a different node. */
> > -		if (!is_mounted)
> > +		if (lgfs2_find_mnt(opts.device, &isdev, &mnt) || mnt == NULL)
> >   			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)
> > +		if (!hasmntopt(mnt, MNTOPT_RO))
> >   			goto mount_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
> > diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
> > index f864a08..993f605 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"
> > @@ -714,7 +715,7 @@ 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_find_mnt(const char *path, int *isdev, 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..6bc95a9 100644
> > --- a/gfs2/libgfs2/misc.c
> > +++ b/gfs2/libgfs2/misc.c
> > @@ -102,20 +102,27 @@ int compute_constants(struct gfs2_sbd *sdp)
> >   	return 0;
> >   }
> >
> > -int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
> > +/**
> > + * lgfs2_find_mnt - Find a mounted file system relating to a path.
> > + * path:  The path
> > + * isdev: Will be non-zero if the path is the device or zero if the path is the mount directory
> > + * mnt:   When the return value is 0, this will either be NULL for "not found" or
> > + *        it will point to the found mount point.
> > + * Returns 0 to indicate a successful scan of the mount points or non-zero on error with errno set.
> > + *         On success, *mnt will be NULL or a valid pointer as noted above.
> > + */
> > +int lgfs2_find_mnt(const char *path, int *isdev, struct mntent **mnt)
> >   {
> >   	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;
> > +		return 1;
> >   	}
> > -	if (stat(path_name, &st_buf) == 0) {
> > +	if (stat(path, &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;
> > @@ -125,42 +132,46 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
> >   			file_ino = st_buf.st_ino;
> >   		}
> >   	}
> > -	while ((mnt = getmntent (fp)) != NULL) {
> > +	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 */
> > +		if (strcmp(path, (*mnt)->mnt_fsname) == 0) {
> > +			*isdev = 1;
> >   			break;
> >   		}
> > -		if (strcmp(path_name, mnt->mnt_dir) == 0) {
> > -			strcpy(device_name, mnt->mnt_fsname); /* fix it */
> > +		if (strcmp(path, (*mnt)->mnt_dir) == 0) {
> > +			*isdev = 0;
> >   			break;
> >   		}
> > -		if (stat(mnt->mnt_fsname, &st_buf) == 0) {
> > +		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))
> > +				if (file_rdev && (file_rdev == st_buf.st_rdev)) {
> > +					*isdev = 1;
> >   					break;
> > +				}
> >   #endif  /* __GNU__ */
> >   			} else {
> >   				if (file_dev && ((file_dev == st_buf.st_dev) &&
> > -						 (file_ino == st_buf.st_ino)))
> > +						 (file_ino == st_buf.st_ino))) {
> > +					*isdev = 0;
> >   					break;
> > +				}
> >   			}
> >   		}
> >   	}
> >   	endmntent (fp);
> > -	if (mnt == NULL)
> > -		return 0;
> > -	if (stat(mnt->mnt_dir, &st_buf) < 0) {
> > +	if (*mnt == NULL)
> > +		return 0; /* Success. Answer is no. */
> > +	if (stat((*mnt)->mnt_dir, &st_buf) < 0) {
> >   		if (errno == ENOENT)
> > -			return 0;
> > +			return 1;
> >   	}
> >   	/* 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 */
> > +	if (file_rdev && (st_buf.st_dev != file_rdev)) {
> > +		errno = EINVAL;
> > +		return 1;
> > +	}
> > +	return 0; /* Success. Answer is yes. */
> >   }
> >
> >   static int lock_for_admin(struct gfs2_sbd *sdp)
> > diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
> > index 5db91d9..4c8807e 100644
> > --- a/gfs2/mkfs/main_grow.c
> > +++ b/gfs2/mkfs/main_grow.c
> > @@ -323,7 +323,6 @@ main_grow(int argc, char *argv[])
> >   	int rgcount, rindex_fd;
> >   	char rindex_name[PATH_MAX];
> >   	int error = EXIT_SUCCESS;
> > -	int ro_mnt = 0;
> >
> >   	memset(sdp, 0, sizeof(struct gfs2_sbd));
> >   	sdp->bsize = GFS2_DEFAULT_BSIZE;
> > @@ -335,16 +334,29 @@ main_grow(int argc, char *argv[])
> >   	
> >   	while ((argc - optind) > 0) {
> >   		int sane;
> > +		int isdev = 0;
> > +		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);
> > +		if (lgfs2_find_mnt(argv[optind], &isdev, &mnt)) {
> > +			perror(argv[optind]);
> >   			exit(EXIT_FAILURE);
> >   		}
> >
> > +		if (mnt == NULL) {
> > +			fprintf(stderr, "%s: not a mounted gfs2 file system\n", argv[optind++]);
> > +			continue;
> > +		}
> > +
> > +		if (isdev) {
> > +			strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
> > +			sdp->path_name = mnt->mnt_dir;
> > +		} else {
> > +			strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
> > +			sdp->path_name = argv[optind];
> > +		}
> > +		optind++;
> > +
> >   		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
> >   		if (sdp->path_fd < 0){
> >   			perror(sdp->path_name);
> > diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
> > index b6cd8e4..9240838 100644
> > --- a/gfs2/mkfs/main_jadd.c
> > +++ b/gfs2/mkfs/main_jadd.c
> > @@ -490,8 +490,9 @@ 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;
> > +	int isdev;
> >
> >   	memset(sdp, 0, sizeof(struct gfs2_sbd));
> >   	sdp->jsize = GFS2_DEFAULT_JSIZE;
> > @@ -500,14 +501,26 @@ 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){
> > +
> > +	if (lgfs2_find_mnt(sdp->path_name, &isdev, &mnt)) {
> >   		perror(sdp->path_name);
> >   		exit(EXIT_FAILURE);
> >   	}
> >
> > -	if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
> > +	if (mnt == NULL) {
> > +		fprintf(stderr, "%s: not a mounted gfs2 file system\n", sdp->path_name);
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	if (isdev) {
> > +		strncpy(sdp->device_name, sdp->path_name, PATH_MAX - 1);
> > +		sdp->path_name = mnt->mnt_dir;
> > +	} else {
> > +		strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
> > +	}
> > +
> > +	sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
> > +	if (sdp->path_fd < 0){
> >   		perror(sdp->path_name);
> >   		exit(EXIT_FAILURE);
> >   	}
> >
> 

[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it

[Andrew Price]

On 05/11/13 11:46, Steven Whitehouse wrote:
> Hi,
>
> Since the path has already been transformed into an fd

This is a false assumption. For example, in fsck.gfs2's initialize() it 
is called in order to find out whether the mount point (if any) is 
read-only after open(path, O_RDWR | O_EXCL) fails so we don't have the 
fd beforehand in that case. In the cases where we do have an fd it's not 
guaranteed to be the correct one (e.g. user gives us the mount point but 
we need to operate on the device).

>, we should be
> using the fd rather than sending the path to lgfs2_find_mnt in order to
> avoid any possible races and also issues wrt device names being
> different,

There's a dependency loop here:

1. To get the correct fd to work with we need to get the right mntent
2. To get the right mntent we need to know the stat info
3. To get the stat info without races we need to fstat the correct fd 
(goto 1)

So I'm really not convinced that we can improve this code much more. 
Even if there's a way to solve the loop without possible races, we'd 
still need to send the path to lgfs2_find_mnt because we need to compare 
it to the mount points and device names of each entry in /proc/mounts.

Btw, if we can solve it we should send a patch for e2fsck because it's 
where the original is_pathname_mounted code came from and it's been 
using the same algorithm for over ten years:

http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/ismounted.c#n39 
:)

Andy

> Steve.
>
> On Mon, 2013-11-04 at 15:38 +0000, Andrew Price wrote:
>> This one fell off my radar somehow. Reviews appreciated.
>>
>> Andy
>>
>> On 12/10/13 23:14, Andrew Price wrote:
>>> is_pathname_mounted is renamed lgfs2_find_mnt and now returns more
>>> context through the isdev and mnt parameters. It now only takes one path
>>> and returns whether it is the name of the device or the mount point
>>> using *isdev. It also sets *mnt to the struct mntent found by
>>> getmntent() so that the caller can check its contents itself.
>>>
>>> Callers have been updated and a bug where gfs2_grow wasn't working when
>>> the device name was provided by the user has been fixed.
>>>
>>> Signed-off-by: Andrew Price <anprice@redhat.com>
>>> ---
>>>    gfs2/fsck/initialize.c |  9 ++++-----
>>>    gfs2/libgfs2/libgfs2.h |  3 ++-
>>>    gfs2/libgfs2/misc.c    | 55 ++++++++++++++++++++++++++++++--------------------
>>>    gfs2/mkfs/main_grow.c  | 24 ++++++++++++++++------
>>>    gfs2/mkfs/main_jadd.c  | 23 ++++++++++++++++-----
>>>    5 files changed, 75 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
>>> index 6612fe7..1cc15ec 100644
>>> --- a/gfs2/fsck/initialize.c
>>> +++ b/gfs2/fsck/initialize.c
>>> @@ -1475,7 +1475,8 @@ 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;
>>> +		int isdev;
>>> +		struct mntent *mnt;
>>>
>>>    		if (open_flag == O_RDONLY || errno != EBUSY) {
>>>    			log_crit( _("Unable to open device: %s\n"),
>>> @@ -1490,18 +1491,16 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
>>>    		   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.
>>>    		   This protects against cases where the file system is LVM
>>>    		   and perhaps mounted on a different node. */
>>> -		if (!is_mounted)
>>> +		if (lgfs2_find_mnt(opts.device, &isdev, &mnt) || mnt == NULL)
>>>    			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)
>>> +		if (!hasmntopt(mnt, MNTOPT_RO))
>>>    			goto mount_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
>>> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
>>> index f864a08..993f605 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"
>>> @@ -714,7 +715,7 @@ 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_find_mnt(const char *path, int *isdev, 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..6bc95a9 100644
>>> --- a/gfs2/libgfs2/misc.c
>>> +++ b/gfs2/libgfs2/misc.c
>>> @@ -102,20 +102,27 @@ int compute_constants(struct gfs2_sbd *sdp)
>>>    	return 0;
>>>    }
>>>
>>> -int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
>>> +/**
>>> + * lgfs2_find_mnt - Find a mounted file system relating to a path.
>>> + * path:  The path
>>> + * isdev: Will be non-zero if the path is the device or zero if the path is the mount directory
>>> + * mnt:   When the return value is 0, this will either be NULL for "not found" or
>>> + *        it will point to the found mount point.
>>> + * Returns 0 to indicate a successful scan of the mount points or non-zero on error with errno set.
>>> + *         On success, *mnt will be NULL or a valid pointer as noted above.
>>> + */
>>> +int lgfs2_find_mnt(const char *path, int *isdev, struct mntent **mnt)
>>>    {
>>>    	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;
>>> +		return 1;
>>>    	}
>>> -	if (stat(path_name, &st_buf) == 0) {
>>> +	if (stat(path, &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;
>>> @@ -125,42 +132,46 @@ int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
>>>    			file_ino = st_buf.st_ino;
>>>    		}
>>>    	}
>>> -	while ((mnt = getmntent (fp)) != NULL) {
>>> +	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 */
>>> +		if (strcmp(path, (*mnt)->mnt_fsname) == 0) {
>>> +			*isdev = 1;
>>>    			break;
>>>    		}
>>> -		if (strcmp(path_name, mnt->mnt_dir) == 0) {
>>> -			strcpy(device_name, mnt->mnt_fsname); /* fix it */
>>> +		if (strcmp(path, (*mnt)->mnt_dir) == 0) {
>>> +			*isdev = 0;
>>>    			break;
>>>    		}
>>> -		if (stat(mnt->mnt_fsname, &st_buf) == 0) {
>>> +		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))
>>> +				if (file_rdev && (file_rdev == st_buf.st_rdev)) {
>>> +					*isdev = 1;
>>>    					break;
>>> +				}
>>>    #endif  /* __GNU__ */
>>>    			} else {
>>>    				if (file_dev && ((file_dev == st_buf.st_dev) &&
>>> -						 (file_ino == st_buf.st_ino)))
>>> +						 (file_ino == st_buf.st_ino))) {
>>> +					*isdev = 0;
>>>    					break;
>>> +				}
>>>    			}
>>>    		}
>>>    	}
>>>    	endmntent (fp);
>>> -	if (mnt == NULL)
>>> -		return 0;
>>> -	if (stat(mnt->mnt_dir, &st_buf) < 0) {
>>> +	if (*mnt == NULL)
>>> +		return 0; /* Success. Answer is no. */
>>> +	if (stat((*mnt)->mnt_dir, &st_buf) < 0) {
>>>    		if (errno == ENOENT)
>>> -			return 0;
>>> +			return 1;
>>>    	}
>>>    	/* 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 */
>>> +	if (file_rdev && (st_buf.st_dev != file_rdev)) {
>>> +		errno = EINVAL;
>>> +		return 1;
>>> +	}
>>> +	return 0; /* Success. Answer is yes. */
>>>    }
>>>
>>>    static int lock_for_admin(struct gfs2_sbd *sdp)
>>> diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
>>> index 5db91d9..4c8807e 100644
>>> --- a/gfs2/mkfs/main_grow.c
>>> +++ b/gfs2/mkfs/main_grow.c
>>> @@ -323,7 +323,6 @@ main_grow(int argc, char *argv[])
>>>    	int rgcount, rindex_fd;
>>>    	char rindex_name[PATH_MAX];
>>>    	int error = EXIT_SUCCESS;
>>> -	int ro_mnt = 0;
>>>
>>>    	memset(sdp, 0, sizeof(struct gfs2_sbd));
>>>    	sdp->bsize = GFS2_DEFAULT_BSIZE;
>>> @@ -335,16 +334,29 @@ main_grow(int argc, char *argv[])
>>>    	
>>>    	while ((argc - optind) > 0) {
>>>    		int sane;
>>> +		int isdev = 0;
>>> +		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);
>>> +		if (lgfs2_find_mnt(argv[optind], &isdev, &mnt)) {
>>> +			perror(argv[optind]);
>>>    			exit(EXIT_FAILURE);
>>>    		}
>>>
>>> +		if (mnt == NULL) {
>>> +			fprintf(stderr, "%s: not a mounted gfs2 file system\n", argv[optind++]);
>>> +			continue;
>>> +		}
>>> +
>>> +		if (isdev) {
>>> +			strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
>>> +			sdp->path_name = mnt->mnt_dir;
>>> +		} else {
>>> +			strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
>>> +			sdp->path_name = argv[optind];
>>> +		}
>>> +		optind++;
>>> +
>>>    		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
>>>    		if (sdp->path_fd < 0){
>>>    			perror(sdp->path_name);
>>> diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
>>> index b6cd8e4..9240838 100644
>>> --- a/gfs2/mkfs/main_jadd.c
>>> +++ b/gfs2/mkfs/main_jadd.c
>>> @@ -490,8 +490,9 @@ 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;
>>> +	int isdev;
>>>
>>>    	memset(sdp, 0, sizeof(struct gfs2_sbd));
>>>    	sdp->jsize = GFS2_DEFAULT_JSIZE;
>>> @@ -500,14 +501,26 @@ 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){
>>> +
>>> +	if (lgfs2_find_mnt(sdp->path_name, &isdev, &mnt)) {
>>>    		perror(sdp->path_name);
>>>    		exit(EXIT_FAILURE);
>>>    	}
>>>
>>> -	if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
>>> +	if (mnt == NULL) {
>>> +		fprintf(stderr, "%s: not a mounted gfs2 file system\n", sdp->path_name);
>>> +		exit(EXIT_FAILURE);
>>> +	}
>>> +
>>> +	if (isdev) {
>>> +		strncpy(sdp->device_name, sdp->path_name, PATH_MAX - 1);
>>> +		sdp->path_name = mnt->mnt_dir;
>>> +	} else {
>>> +		strncpy(sdp->device_name, mnt->mnt_fsname, PATH_MAX - 1);
>>> +	}
>>> +
>>> +	sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
>>> +	if (sdp->path_fd < 0){
>>>    		perror(sdp->path_name);
>>>    		exit(EXIT_FAILURE);
>>>    	}
>>>
>>
>
>

[Cluster-devel] [PATCH] libgfs2: Return more context from is_pathname_mounted and rename it

[Steven Whitehouse]

Hi,

On Tue, 2013-11-05 at 14:04 +0000, Andrew Price wrote:
> On 05/11/13 11:46, Steven Whitehouse wrote:
> > Hi,
> >
> > Since the path has already been transformed into an fd
> 
> This is a false assumption. For example, in fsck.gfs2's initialize() it 
> is called in order to find out whether the mount point (if any) is 
> read-only after open(path, O_RDWR | O_EXCL) fails so we don't have the 
> fd beforehand in that case. In the cases where we do have an fd it's not 
> guaranteed to be the correct one (e.g. user gives us the mount point but 
> we need to operate on the device).
> 
Well fsck has slightly different requirements - it wants to ensure that
the fs is not mounted, whereas grow wants to know that it is mounted. It
may be worth splitting the two cases if they have other differences as
well.

> >, we should be
> > using the fd rather than sending the path to lgfs2_find_mnt in order to
> > avoid any possible races and also issues wrt device names being
> > different,
> 
> There's a dependency loop here:
> 
> 1. To get the correct fd to work with we need to get the right mntent
> 2. To get the right mntent we need to know the stat info
> 3. To get the stat info without races we need to fstat the correct fd 
> (goto 1)
> 
> So I'm really not convinced that we can improve this code much more. 
> Even if there's a way to solve the loop without possible races, we'd 
> still need to send the path to lgfs2_find_mnt because we need to compare 
> it to the mount points and device names of each entry in /proc/mounts.
> 
> Btw, if we can solve it we should send a patch for e2fsck because it's 
> where the original is_pathname_mounted code came from and it's been 
> using the same algorithm for over ten years:
> 
> http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/ismounted.c#n39 
> :)
> 
> Andy
> 
Thats ok I think. What we should be doing is to open an fd for whatever
string the user has given us - in the fsck case, if O_RDWR fails, we can
try O_RDONLY. Then we can fstat to find out what it is - device, file or
directory and then look to see if we can find the mntent based on that.
Open each dev in turn and compare dev numbers if we are looking for a
dev, otherwise we know the original fd refers to a mount point and we
can compare the strings, or something along those lines.

So yes, we may need to send both the path and the fd to lfs2_find_mnt()
or maybe we should just write a/some function(s) which do the whole task
of taking the string and returning a device fd, and feeding it some
flags which tells the function whether (or not) we want to have a device
that is mounted or not, if that makes sense in the cases that we have,

Steve.