From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Fri, 09 Aug 2013 10:41:17 +0100 Subject: [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string In-Reply-To: <1375981344-30943-1-git-send-email-anprice@redhat.com> References: <1375981344-30943-1-git-send-email-anprice@redhat.com> Message-ID: <1376041277.2718.3.camel@menhir> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 > --- > 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); > }