[Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Thu, 2013-08-08 at 18:02 +0100, Andrew Price wrote:
> 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,

Steve.

> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  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(-)
> 
> diff --git a/gfs2/fsck/initialize.c b/gfs2/fsck/initialize.c
> index b45ae08..6612fe7 100644
> --- a/gfs2/fsck/initialize.c
> +++ b/gfs2/fsck/initialize.c
> @@ -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. */
> diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
> index fe7129a..e994d3b 100644
> --- a/gfs2/libgfs2/libgfs2.h
> +++ b/gfs2/libgfs2/libgfs2.h
> @@ -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);
> diff --git a/gfs2/libgfs2/misc.c b/gfs2/libgfs2/misc.c
> index c05a770..6f2dbdb 100644
> --- a/gfs2/libgfs2/misc.c
> +++ b/gfs2/libgfs2/misc.c
> @@ -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;
> diff --git a/gfs2/mkfs/main_grow.c b/gfs2/mkfs/main_grow.c
> index 73c7f84..d324af9 100644
> --- a/gfs2/mkfs/main_grow.c
> +++ b/gfs2/mkfs/main_grow.c
> @@ -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){
> diff --git a/gfs2/mkfs/main_jadd.c b/gfs2/mkfs/main_jadd.c
> index 58fb046..2646829 100644
> --- a/gfs2/mkfs/main_jadd.c
> +++ b/gfs2/mkfs/main_jadd.c
> @@ -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);
>  	}