gfs2_grow: Don't try to open an empty string

sdp->device_name wasn't getting set in gfs2_grow so is_gfs2() (called via check_for_gfs2()) fails to open "" and so we get an ENOENT.

This fixes the open("") by setting sdp->device_name before passing it to is_pathname_mounted(), which has been updated to make it more clear which args will be modified by it.

is_gfs2() and check_for_gfs2() have also been removed from libgfs2 as these were the only places they were being called and they aren't
needed: superblock checking is done further along via other functions calling check_sb().

Signed-off-by Andrew Price <[email protected]>

---
 gfs2/fsck/initialize.c |  2 +-
 gfs2/libgfs2/libgfs2.h |  4 +---
 gfs2/libgfs2/misc.c    | 42 ++++++------------------------------------
 gfs2/mkfs/main_grow.c  | 10 +++++++---
 gfs2/mkfs/main_jadd.c  |  3 ++-
 5 files changed, 17 insertions(+), 44 deletions(-)

gfs2/fsck/initialize.c +1 -1

index b45ae08..6612fe7 100644
@@ -1491,7 +1491,7 @@ int initialize(struct gfs2_sbd *sdp, int force_check, int preen,
 		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, &ro);
+		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. */

gfs2/libgfs2/libgfs2.h +1 -3

index fe7129a..e994d3b 100644
@@ -716,11 +716,9 @@ extern void decrease_verbosity(void);
 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(struct gfs2_sbd *sdp, int *ro_mount);
-extern int is_gfs2(struct gfs2_sbd *sdp);
+extern int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount);
 extern int find_gfs2_meta(struct gfs2_sbd *sdp);
 extern int dir_exists(const char *dir);
-extern int check_for_gfs2(struct gfs2_sbd *sdp);
 extern int mount_gfs2_meta(struct gfs2_sbd *sdp);
 extern void cleanup_metafs(struct gfs2_sbd *sdp);
 extern int set_sysfs(const char *fsname, const char *filename, const char *val);

gfs2/libgfs2/misc.c +6 -36

index c05a770..6f2dbdb 100644
@@ -99,7 +99,7 @@ int compute_constants(struct gfs2_sbd *sdp)
 	return 0;
 }
 
-int is_pathname_mounted(struct gfs2_sbd *sdp, int *ro_mount)
+int is_pathname_mounted(char *path_name, char *device_name, int *ro_mount)
 {
 	FILE *fp;
 	struct mntent *mnt;

@@ -112,7 +112,7 @@ int is_pathname_mounted(struct gfs2_sbd *sdp, int *ro_mount)
 		perror("open: /proc/mounts");
 		return 0;
 	}
-	if (stat(sdp->path_name, &st_buf) == 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;

@@ -124,12 +124,12 @@ int is_pathname_mounted(struct gfs2_sbd *sdp, int *ro_mount)
 	}
 	while ((mnt = getmntent (fp)) != NULL) {
 		/* Check if they specified the device instead of mnt point */
-		if (strcmp(sdp->device_name, mnt->mnt_fsname) == 0) {
-			strcpy(sdp->path_name, mnt->mnt_dir); /* fix it */
+		if (strcmp(device_name, mnt->mnt_fsname) == 0) {
+			strcpy(path_name, mnt->mnt_dir); /* fix it */
 			break;
 		}
-		if (strcmp(sdp->path_name, mnt->mnt_dir) == 0) {
-			strcpy(sdp->device_name, mnt->mnt_fsname); /* fix it */
+		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) {

@@ -160,36 +160,6 @@ int is_pathname_mounted(struct gfs2_sbd *sdp, int *ro_mount)
 	return 1; /* mounted */
 }
 
-int is_gfs2(struct gfs2_sbd *sdp)
-{
-	int fd, rc;
-	struct gfs2_sb sb;
-
-	fd = open(sdp->device_name, O_RDWR);
-	if (fd < 0)
-		return 0;
-
-	rc = 0;
-	if (lseek(fd, GFS2_SB_ADDR * GFS2_BASIC_BLOCK, SEEK_SET) >= 0 &&
-	    read(fd, &sb, sizeof(sb)) == sizeof(sb) &&
-	    be32_to_cpu(sb.sb_header.mh_magic) == GFS2_MAGIC &&
-	    be32_to_cpu(sb.sb_header.mh_type) == GFS2_METATYPE_SB)
-		rc = 1;
-	close(fd);
-	return rc;
-}
-
-int check_for_gfs2(struct gfs2_sbd *sdp)
-{
-	int ro;
-
-	if (!is_pathname_mounted(sdp, &ro))
-		return -1;
-	if (!is_gfs2(sdp))
-		return -1;
-	return 0;
-}
-
 static int lock_for_admin(struct gfs2_sbd *sdp)
 {
 	int error;

gfs2/mkfs/main_grow.c +7 -3

index 73c7f84..d324af9 100644
@@ -323,6 +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;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->bsize = GFS2_DEFAULT_BSIZE;

@@ -336,17 +337,20 @@ main_grow(int argc, char *argv[])
 		int sane;
 		struct rgrp_tree *last_rgrp;
 
+		strncpy(sdp->device_name, argv[optind], PATH_MAX - 1);
 		sdp->path_name = argv[optind++];
-		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
-		if (sdp->path_fd < 0){
+
+		if ((!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt))) {
 			perror(sdp->path_name);
 			exit(EXIT_FAILURE);
 		}
 
-		if (check_for_gfs2(sdp)) {
+		sdp->path_fd = open(sdp->path_name, O_RDONLY | O_CLOEXEC);
+		if (sdp->path_fd < 0){
 			perror(sdp->path_name);
 			exit(EXIT_FAILURE);
 		}
+
 		sdp->device_fd = open(sdp->device_name,
 				      (test ? O_RDONLY : O_RDWR) | O_CLOEXEC);
 		if (sdp->device_fd < 0){

gfs2/mkfs/main_jadd.c +2 -1

index 58fb046..2646829 100644
@@ -491,6 +491,7 @@ void main_jadd(int argc, char *argv[])
 {
 	struct gfs2_sbd sbd, *sdp = &sbd;
 	unsigned int total;
+	int ro_mnt = 0;
 
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->jsize = GFS2_DEFAULT_JSIZE;

@@ -506,7 +507,7 @@ void main_jadd(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	if (check_for_gfs2(sdp)) {
+	if (!is_pathname_mounted(sdp->path_name, sdp->device_name, &ro_mnt)) {
 		perror(sdp->path_name);
 		exit(EXIT_FAILURE);
 	}

Hi,

Quoted 12 lines · Andrew Price

sdp->device_name wasn't getting set in gfs2_grow so is_gfs2() (called via check_for_gfs2()) fails to open "" and so we get an ENOENT.

This fixes the open("") by setting sdp->device_name before passing it to is_pathname_mounted(), which has been updated to make it more clear which args will be modified by it.

is_gfs2() and check_for_gfs2() have also been removed from libgfs2 as these were the only places they were being called and they aren't
needed: superblock checking is done further along via other functions calling check_sb().

Definitely an improvement over what we had before, but I wonder whether we can do better still. Is the fd always open before we call is_pathname_mounted() I wonder? If so we should be able to just pass the fd to it which reduces the possibility for races I think, and may also simplify things a bit more,

Quoted 15 lines · Steven Whitehouse

Hi,

Quoted 12 lines · Andrew Price

sdp->device_name wasn't getting set in gfs2_grow so is_gfs2() (called via check_for_gfs2()) fails to open "" and so we get an ENOENT.

This fixes the open("") by setting sdp->device_name before passing it to is_pathname_mounted(), which has been updated to make it more clear which args will be modified by it.

is_gfs2() and check_for_gfs2() have also been removed from libgfs2 as these were the only places they were being called and they aren't
needed: superblock checking is done further along via other functions calling check_sb().

Quoted 5 lines

Definitely an improvement over what we had before, but I wonder whether we can do better still. Is the fd always open before we call is_pathname_mounted() I wonder? If so we should be able to just pass the fd to it which reduces the possibility for races I think, and may also simplify things a bit more,

Well the problem here is that we don't know whether the path is a device or a mount point (gfs2_{grow,jadd} accept both) beforehand so any open before is_pathname_mounted() is called is going to be a guess.

I think we probably can improve on this function in the process of making libgfs2 more fd-centric but it'll take a bit more work so I'll come back to that later and push this patch independently as it solves a problem that's currently holding up testing.

Cheers,
Andy