* [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string
@ 2013-08-08 17:02 Andrew Price
2013-08-09 9:41 ` Steven Whitehouse
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Price @ 2013-08-08 17:02 UTC (permalink / raw)
To: cluster-devel.redhat.com
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 <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);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string
2013-08-08 17:02 [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string Andrew Price
@ 2013-08-09 9:41 ` Steven Whitehouse
2013-08-13 9:04 ` Andrew Price
0 siblings, 1 reply; 4+ messages in thread
From: Steven Whitehouse @ 2013-08-09 9:41 UTC (permalink / raw)
To: cluster-devel.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);
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string
2013-08-09 9:41 ` Steven Whitehouse
@ 2013-08-13 9:04 ` Andrew Price
2013-08-13 9:12 ` Steven Whitehouse
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Price @ 2013-08-13 9:04 UTC (permalink / raw)
To: cluster-devel.redhat.com
On 09/08/13 10:41, Steven Whitehouse wrote:
> 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,
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string
2013-08-13 9:04 ` Andrew Price
@ 2013-08-13 9:12 ` Steven Whitehouse
0 siblings, 0 replies; 4+ messages in thread
From: Steven Whitehouse @ 2013-08-13 9:12 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi,
On Tue, 2013-08-13 at 10:04 +0100, Andrew Price wrote:
> On 09/08/13 10:41, Steven Whitehouse wrote:
> > 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,
>
> 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.
>
It will be a guess, but at least once it is open, the object that is
referenced cannot change. We can then verify what we got and decide what
to do next with it, safe in the knowledge that nobody can change a
symlink (for example) under us and give us the wrong results later on.
> 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
Yes, I think thats a good approach,
Steve.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-13 9:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-08 17:02 [Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string Andrew Price
2013-08-09 9:41 ` Steven Whitehouse
2013-08-13 9:04 ` Andrew Price
2013-08-13 9:12 ` Steven Whitehouse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).