Eric Sandeen
2013-Mar-11 22:27 UTC
Re: [PATCH 4/4] btrfs-progs: rework get_fs_info to remove side effects
On 3/11/13 6:13 PM, Eric Sandeen wrote:> get_fs_info() has been silently switching from a device to a mounted > path as needed; the caller''s filehandle was unexpectedly closed & > reopened outside the caller''s scope. Not so great. > > The callers do want "fdmnt" to be the filehandle for the mount point > in all cases, though - the various ioctls act on this (not on an fd > for the device). But switching it in the local scope of get_fs_info > is incorrect; it just so happens that *usually* the fd number is > unchanged. > > So - use the new helpers to detect when an argument is a block > device, and open the the mounted path more obviously / explicitly > for ioctl use, storing the filehandle in fdmnt. > > Then, in get_fs_info, ignore the fd completely, and use the path on > the argument to determine if the caller wanted to act on just that > device, or on all devices for the filesystem. > > Affects those commands which are documented to accept either > a block device or a path:Following my tradition I''ll (immediately) self-nak this one for now. After I sent this I thought to test: # mkfs.btrfs /dev/sdb1 /dev/sdb2; mount /dev/sdb1 /mnt/test; btrfs stats /dev/sdb2 after I tested it, and that fails where it used to work. So a) we could use a test for this, and b) I broke something If the overall idea of the change seems decent, I''ll get it fixed up after I sort out what I broke. :/ -Eric> * btrfs device stats > * btrfs replace start > * btrfs scrub start > * btrfs scrub status > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > cmds-device.c | 5 ++- > cmds-replace.c | 6 +++- > cmds-scrub.c | 10 ++++--- > utils.c | 73 +++++++++++++++++++++++++++++++++++++++---------------- > utils.h | 2 +- > 5 files changed, 66 insertions(+), 30 deletions(-) > > diff --git a/cmds-device.c b/cmds-device.c > index 58df6da..41e79d3 100644 > --- a/cmds-device.c > +++ b/cmds-device.c > @@ -321,13 +321,14 @@ static int cmd_dev_stats(int argc, char **argv) > > path = argv[optind]; > > - fdmnt = open_file_or_dir(path); > + fdmnt = open_path_or_dev_mnt(path); > + > if (fdmnt < 0) { > fprintf(stderr, "ERROR: can''t access ''%s''\n", path); > return 12; > } > > - ret = get_fs_info(fdmnt, path, &fi_args, &di_args); > + ret = get_fs_info(path, &fi_args, &di_args); > if (ret) { > fprintf(stderr, "ERROR: getting dev info for devstats failed: " > "%s\n", strerror(-ret)); > diff --git a/cmds-replace.c b/cmds-replace.c > index 10030f6..6397bb5 100644 > --- a/cmds-replace.c > +++ b/cmds-replace.c > @@ -168,7 +168,9 @@ static int cmd_start_replace(int argc, char **argv) > if (check_argc_exact(argc - optind, 3)) > usage(cmd_start_replace_usage); > path = argv[optind + 2]; > - fdmnt = open_file_or_dir(path); > + > + fdmnt = open_path_or_dev_mnt(path); > + > if (fdmnt < 0) { > fprintf(stderr, "ERROR: can''t access \"%s\": %s\n", > path, strerror(errno)); > @@ -215,7 +217,7 @@ static int cmd_start_replace(int argc, char **argv) > } > start_args.start.srcdevid = (__u64)atoi(srcdev); > > - ret = get_fs_info(fdmnt, path, &fi_args, &di_args); > + ret = get_fs_info(path, &fi_args, &di_args); > if (ret) { > fprintf(stderr, "ERROR: getting dev info for devstats failed: " > "%s\n", strerror(-ret)); > diff --git a/cmds-scrub.c b/cmds-scrub.c > index e5fccc7..52264f1 100644 > --- a/cmds-scrub.c > +++ b/cmds-scrub.c > @@ -1101,13 +1101,14 @@ static int scrub_start(int argc, char **argv, int resume) > > path = argv[optind]; > > - fdmnt = open_file_or_dir(path); > + fdmnt = open_path_or_dev_mnt(path); > + > if (fdmnt < 0) { > ERR(!do_quiet, "ERROR: can''t access ''%s''\n", path); > return 12; > } > > - ret = get_fs_info(fdmnt, path, &fi_args, &di_args); > + ret = get_fs_info(path, &fi_args, &di_args); > if (ret) { > ERR(!do_quiet, "ERROR: getting dev info for scrub failed: " > "%s\n", strerror(-ret)); > @@ -1558,13 +1559,14 @@ static int cmd_scrub_status(int argc, char **argv) > > path = argv[optind]; > > - fdmnt = open_file_or_dir(path); > + fdmnt = open_path_or_dev_mnt(path); > + > if (fdmnt < 0) { > fprintf(stderr, "ERROR: can''t access to ''%s''\n", path); > return 12; > } > > - ret = get_fs_info(fdmnt, path, &fi_args, &di_args); > + ret = get_fs_info(path, &fi_args, &di_args); > if (ret) { > fprintf(stderr, "ERROR: getting dev info for scrub failed: " > "%s\n", strerror(-ret)); > diff --git a/utils.c b/utils.c > index 4bf457f..27cec56 100644 > --- a/utils.c > +++ b/utils.c > @@ -717,7 +717,7 @@ int open_path_or_dev_mnt(const char *path) > errno = EINVAL; > return -1; > } > - fdmnt = open(mp, O_RDWR); > + fdmnt = open_file_or_dir(mp); > } else > fdmnt = open_file_or_dir(path); > > @@ -1544,9 +1544,20 @@ int get_device_info(int fd, u64 devid, > return ret ? -errno : 0; > } > > -int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, > +/* > + * For a given path, fill in the ioctl fs_ and info_ args. > + * If the path is a btrfs mountpoint, fill info for all devices. > + * If the path is a btrfs device, fill in only that device. > + * > + * The path provided must be either on a mounted btrfs fs, > + * or be a mounted btrfs device. > + * > + * Returns 0 on success, or a negative errno. > + */ > +int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args, > struct btrfs_ioctl_dev_info_args **di_ret) > { > + int fd = -1; > int ret = 0; > int ndevs = 0; > int i = 1; > @@ -1556,33 +1567,50 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, > > memset(fi_args, 0, sizeof(*fi_args)); > > - ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args); > - if (ret && (errno == EINVAL || errno == ENOTTY)) { > - /* path is not a mounted btrfs. Try if it''s a device */ > + if (is_block_device(path)) { > + /* Ensure it''s mounted, then set path to the mountpoint */ > + fd = open(path, O_RDONLY); > + if (fd < 0) { > + ret = -errno; > + fprintf(stderr, "Couldn''t open %s: %s\n", > + path, strerror(errno)); > + goto out; > + } > ret = check_mounted_where(fd, path, mp, sizeof(mp), > &fs_devices_mnt); > - if (!ret) > - return -EINVAL; > - if (ret < 0) > - return ret; > + if (ret < 0) { > + fprintf(stderr, "Couldn''t get mountpoint for %s: %s\n", > + path, strerror(-ret)); > + goto out; > + } > + path = mp; > + /* Only fill in this one device */ > fi_args->num_devices = 1; > fi_args->max_id = fs_devices_mnt->latest_devid; > i = fs_devices_mnt->latest_devid; > memcpy(fi_args->fsid, fs_devices_mnt->fsid, BTRFS_FSID_SIZE); > - close(fd); > - fd = open_file_or_dir(mp); > - if (fd < 0) > - return -errno; > - } else if (ret) { > - return -errno; > + } > + > + fd = open_file_or_dir(path); > + if (fd < 0) { > + ret = -errno; > + goto out; > + } > + > + ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args); > + if (ret < 0) { > + ret = -errno; > + goto out; > } > > if (!fi_args->num_devices) > - return 0; > + goto out; > > di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args)); > - if (!di_args) > - return -errno; > + if (!di_args) { > + ret = -errno; > + goto out; > + } > > for (; i <= fi_args->max_id; ++i) { > BUG_ON(ndevs >= fi_args->num_devices); > @@ -1590,13 +1618,16 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, > if (ret == -ENODEV) > continue; > if (ret) > - return ret; > + goto out; > ndevs++; > } > > BUG_ON(ndevs == 0); > - > - return 0; > + ret = 0; > +out: > + if (fd != -1) > + close(fd); > + return ret; > } > > #define isoctal(c) (((c) & ~7) == ''0'') > diff --git a/utils.h b/utils.h > index 8e0252b..885b9c5 100644 > --- a/utils.h > +++ b/utils.h > @@ -50,7 +50,7 @@ u64 parse_size(char *s); > int open_file_or_dir(const char *fname); > int get_device_info(int fd, u64 devid, > struct btrfs_ioctl_dev_info_args *di_args); > -int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, > +int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args, > struct btrfs_ioctl_dev_info_args **di_ret); > int get_label(const char *btrfs_dev); > int set_label(const char *btrfs_dev, const char *label); >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The first patch is a trival close of fd on function returns, somehow missed that last go-round. The next 3 are a little more substantial, working to avoid the nasty behavior of get_fs_info, closing & re-opening the callers'' filehandle out of scope, if it needs to switch from device node to mountpoint. (I suppose we could pass in *fd by reference, but this behavior just seems like a wrong, magical side effect for get_fs_info). So instead, the callers use a helper to *always* wind up with the mountpoint opened, and get_fs_info() now *only* - well - only gets fs info. The previous behavior of "if given a device act only on that device; if given a mountpoint act on all devices" should persist; I guess that''s the original intent. It''s really only lightly tested; it should mostly affect: * btrfs device stats * btrfs replace start * btrfs scrub start * btrfs scrub status so any independent sanity testing of that would be great. It''l be nice if/when we get xfstests coverage of some of this to make it easier. :) Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen
2013-Mar-11 23:12 UTC
[PATCH 1/4] btrfs-progs: close fd on return from label get/set functions
Somehow missed these 2 in the last round. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- utils.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/utils.c b/utils.c index f68436d..1c73d67 100644 --- a/utils.c +++ b/utils.c @@ -1217,6 +1217,7 @@ static int set_label_mounted(const char *mount_path, const char *label) return -1; } + close(fd); return 0; } @@ -1274,6 +1275,7 @@ static int get_label_mounted(const char *mount_path) } fprintf(stdout, "%s\n", label); + close(fd); return 0; } -- 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Add 3 new helpers:
* is_block_device(), to test if a path is a block device.
* get_btrfs_mount(), to get the mountpoint of a device,
if mounted.
* open_path_or_dev_mnt(path), to open either the pathname
or, if it''s a mounted btrfs dev, the mountpoint. Useful
for some commands which can take either type of arg.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
utils.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
utils.h | 3 ++
2 files changed, 87 insertions(+), 0 deletions(-)
diff --git a/utils.c b/utils.c
index 1c73d67..4bf457f 100644
--- a/utils.c
+++ b/utils.c
@@ -640,6 +640,90 @@ error:
return ret;
}
+/*
+ * checks if a path is a block device node
+ * Returns negative errno on failure, otherwise
+ * returns 1 for blockdev, 0 for not-blockdev
+ */
+int is_block_device (const char *path) {
+ struct stat statbuf;
+
+ if (stat(path, &statbuf) < 0)
+ return -errno;
+
+ return (S_ISBLK(statbuf.st_mode));
+}
+
+/*
+ * Find the mount point for a mounted device.
+ * On success, returns 0 with mountpoint in *mp.
+ * On failure, returns -errno (not mounted yields -EINVAL)
+ * Is noisy on failures, expects to be given a mounted device.
+ */
+int get_btrfs_mount(const char *dev, char *mp, size_t mp_size) {
+ int ret;
+ int fd = -1;
+
+ ret = is_block_device(dev);
+ if (ret <= 0) {
+ if (!ret) {
+ fprintf(stderr, "%s is not a block device\n", dev);
+ ret = -EINVAL;
+ } else
+ fprintf(stderr, "Could not check %s: %s\n",
+ dev, strerror(-ret));
+ goto out;
+ }
+
+ fd = open(dev, O_RDONLY);
+ if (fd < 0) {
+ ret = -errno;
+ fprintf(stderr, "Could not open %s: %s\n", dev, strerror(errno));
+ goto out;
+ }
+
+ ret = check_mounted_where(fd, dev, mp, mp_size, NULL);
+ if (!ret) {
+ fprintf(stderr, "%s is not a mounted btrfs device\n", dev);
+ ret = -EINVAL;
+ } else /* mounted, all good */
+ ret = 0;
+out:
+ if (fd != -1)
+ close(fd);
+ if (ret)
+ fprintf(stderr, "Could not get mountpoint for %s\n", dev);
+ return ret;
+}
+
+/*
+ * Given a pathname, return a filehandle to:
+ * the original pathname or,
+ * if the pathname is a mounted btrfs device, to its mountpoint.
+ *
+ * On error, return -1, errno should be set.
+ */
+int open_path_or_dev_mnt(const char *path)
+{
+ char mp[BTRFS_PATH_NAME_MAX + 1];
+ int fdmnt;
+
+ if (is_block_device(path)) {
+ int ret;
+
+ ret = get_btrfs_mount(path, mp, sizeof(mp));
+ if (ret < 0) {
+ /* not a mounted btrfs dev */
+ errno = EINVAL;
+ return -1;
+ }
+ fdmnt = open(mp, O_RDWR);
+ } else
+ fdmnt = open_file_or_dir(path);
+
+ return fdmnt;
+}
+
/* checks if a device is a loop device */
int is_loop_device (const char* device) {
struct stat statbuf;
diff --git a/utils.h b/utils.h
index 0b681ed..8e0252b 100644
--- a/utils.h
+++ b/utils.h
@@ -56,6 +56,9 @@ int get_label(const char *btrfs_dev);
int set_label(const char *btrfs_dev, const char *label);
char *__strncpy__null(char *dest, const char *src, size_t n);
+int is_block_device(const char *file);
+int get_btrfs_mount(const char *path, char *mp, size_t mp_size);
+int open_path_or_dev_mnt(const char *path);
int is_swap_device(const char *file);
/* Helper to always get proper size of the destination string */
#define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen
2013-Mar-11 23:13 UTC
[PATCH 3/4] btrfs-progs: don''t open-code mountpoint discovery in scrub cancel
cmd_scrub_cancel had its own mountpoint discovery routine;
just use open_path_or_dev_mnt() for that now.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
cmds-scrub.c | 53 +++++++++++++++++------------------------------------
1 files changed, 17 insertions(+), 36 deletions(-)
diff --git a/cmds-scrub.c b/cmds-scrub.c
index b0fcde6..e5fccc7 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1459,56 +1459,37 @@ static int cmd_scrub_cancel(int argc, char **argv)
{
char *path;
int ret;
- int fdmnt;
- int err;
- char mp[BTRFS_PATH_NAME_MAX + 1];
- struct btrfs_fs_devices *fs_devices_mnt = NULL;
+ int fdmnt = -1;
if (check_argc_exact(argc, 2))
usage(cmd_scrub_cancel_usage);
path = argv[1];
-again:
- fdmnt = open_file_or_dir(path);
- if (fdmnt < 0) {
- perror("ERROR: scrub cancel failed:");
- return 1;
- }
+ fdmnt = open_path_or_dev_mnt(path);
+ if (fdmnt < 0) {
+ fprintf(stderr, "ERROR: could not open %s: %s\n",
+ path, strerror(errno));
+ ret = 1;
+ goto out;
+ }
ret = ioctl(fdmnt, BTRFS_IOC_SCRUB_CANCEL, NULL);
- err = errno;
-
- if (ret && err == EINVAL) {
- /* path is not a btrfs mount point. See if it''s a device. */
- ret = check_mounted_where(fdmnt, path, mp, sizeof(mp),
- &fs_devices_mnt);
- if (ret > 0) {
- /* It''s a mounted btrfs device; retry w/ mountpoint. */
- close(fdmnt);
- path = mp;
- goto again;
- } else {
- /* It''s not a mounted btrfs device either */
- fprintf(stderr,
- "ERROR: %s is not a mounted btrfs device\n",
- path);
- ret = 1;
- err = EINVAL;
- }
- }
- close(fdmnt);
-
- if (ret) {
+ if (ret < 0) {
fprintf(stderr, "ERROR: scrub cancel failed on %s: %s\n", path,
- err == ENOTCONN ? "not running" : strerror(err));
- return 1;
+ errno == ENOTCONN ? "not running" : strerror(errno));
+ ret = 1;
+ goto out;
}
+ ret = 0;
printf("scrub cancelled\n");
- return 0;
+out:
+ if (fdmnt != -1)
+ close(fdmnt);
+ return ret;
}
static const char * const cmd_scrub_resume_usage[] = {
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen
2013-Mar-11 23:13 UTC
[PATCH 4/4] btrfs-progs: rework get_fs_info to remove side effects
get_fs_info() has been silently switching from a device to a mounted
path as needed; the caller''s filehandle was unexpectedly closed &
reopened outside the caller''s scope. Not so great.
The callers do want "fdmnt" to be the filehandle for the mount point
in all cases, though - the various ioctls act on this (not on an fd
for the device). But switching it in the local scope of get_fs_info
is incorrect; it just so happens that *usually* the fd number is
unchanged.
So - use the new helpers to detect when an argument is a block
device, and open the the mounted path more obviously / explicitly
for ioctl use, storing the filehandle in fdmnt.
Then, in get_fs_info, ignore the fd completely, and use the path on
the argument to determine if the caller wanted to act on just that
device, or on all devices for the filesystem.
Affects those commands which are documented to accept either
a block device or a path:
* btrfs device stats
* btrfs replace start
* btrfs scrub start
* btrfs scrub status
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
cmds-device.c | 5 ++-
cmds-replace.c | 6 +++-
cmds-scrub.c | 10 ++++---
utils.c | 73 +++++++++++++++++++++++++++++++++++++++----------------
utils.h | 2 +-
5 files changed, 66 insertions(+), 30 deletions(-)
diff --git a/cmds-device.c b/cmds-device.c
index 58df6da..41e79d3 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -321,13 +321,14 @@ static int cmd_dev_stats(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
fprintf(stderr, "ERROR: can''t access
''%s''\n", path);
return 12;
}
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for devstats failed: "
"%s\n", strerror(-ret));
diff --git a/cmds-replace.c b/cmds-replace.c
index 10030f6..6397bb5 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -168,7 +168,9 @@ static int cmd_start_replace(int argc, char **argv)
if (check_argc_exact(argc - optind, 3))
usage(cmd_start_replace_usage);
path = argv[optind + 2];
- fdmnt = open_file_or_dir(path);
+
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
fprintf(stderr, "ERROR: can''t access \"%s\":
%s\n",
path, strerror(errno));
@@ -215,7 +217,7 @@ static int cmd_start_replace(int argc, char **argv)
}
start_args.start.srcdevid = (__u64)atoi(srcdev);
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for devstats failed: "
"%s\n", strerror(-ret));
diff --git a/cmds-scrub.c b/cmds-scrub.c
index e5fccc7..52264f1 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1101,13 +1101,14 @@ static int scrub_start(int argc, char **argv, int
resume)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
ERR(!do_quiet, "ERROR: can''t access
''%s''\n", path);
return 12;
}
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1558,13 +1559,14 @@ static int cmd_scrub_status(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
fprintf(stderr, "ERROR: can''t access to
''%s''\n", path);
return 12;
}
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
diff --git a/utils.c b/utils.c
index 4bf457f..27cec56 100644
--- a/utils.c
+++ b/utils.c
@@ -717,7 +717,7 @@ int open_path_or_dev_mnt(const char *path)
errno = EINVAL;
return -1;
}
- fdmnt = open(mp, O_RDWR);
+ fdmnt = open_file_or_dir(mp);
} else
fdmnt = open_file_or_dir(path);
@@ -1544,9 +1544,20 @@ int get_device_info(int fd, u64 devid,
return ret ? -errno : 0;
}
-int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
+/*
+ * For a given path, fill in the ioctl fs_ and info_ args.
+ * If the path is a btrfs mountpoint, fill info for all devices.
+ * If the path is a btrfs device, fill in only that device.
+ *
+ * The path provided must be either on a mounted btrfs fs,
+ * or be a mounted btrfs device.
+ *
+ * Returns 0 on success, or a negative errno.
+ */
+int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret)
{
+ int fd = -1;
int ret = 0;
int ndevs = 0;
int i = 1;
@@ -1556,33 +1567,50 @@ int get_fs_info(int fd, char *path, struct
btrfs_ioctl_fs_info_args *fi_args,
memset(fi_args, 0, sizeof(*fi_args));
- ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
- if (ret && (errno == EINVAL || errno == ENOTTY)) {
- /* path is not a mounted btrfs. Try if it''s a device */
+ if (is_block_device(path)) {
+ /* Ensure it''s mounted, then set path to the mountpoint */
+ fd = open(path, O_RDONLY);
+ if (fd < 0) {
+ ret = -errno;
+ fprintf(stderr, "Couldn''t open %s: %s\n",
+ path, strerror(errno));
+ goto out;
+ }
ret = check_mounted_where(fd, path, mp, sizeof(mp),
&fs_devices_mnt);
- if (!ret)
- return -EINVAL;
- if (ret < 0)
- return ret;
+ if (ret < 0) {
+ fprintf(stderr, "Couldn''t get mountpoint for %s: %s\n",
+ path, strerror(-ret));
+ goto out;
+ }
+ path = mp;
+ /* Only fill in this one device */
fi_args->num_devices = 1;
fi_args->max_id = fs_devices_mnt->latest_devid;
i = fs_devices_mnt->latest_devid;
memcpy(fi_args->fsid, fs_devices_mnt->fsid, BTRFS_FSID_SIZE);
- close(fd);
- fd = open_file_or_dir(mp);
- if (fd < 0)
- return -errno;
- } else if (ret) {
- return -errno;
+ }
+
+ fd = open_file_or_dir(path);
+ if (fd < 0) {
+ ret = -errno;
+ goto out;
+ }
+
+ ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
+ if (ret < 0) {
+ ret = -errno;
+ goto out;
}
if (!fi_args->num_devices)
- return 0;
+ goto out;
di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
- if (!di_args)
- return -errno;
+ if (!di_args) {
+ ret = -errno;
+ goto out;
+ }
for (; i <= fi_args->max_id; ++i) {
BUG_ON(ndevs >= fi_args->num_devices);
@@ -1590,13 +1618,16 @@ int get_fs_info(int fd, char *path, struct
btrfs_ioctl_fs_info_args *fi_args,
if (ret == -ENODEV)
continue;
if (ret)
- return ret;
+ goto out;
ndevs++;
}
BUG_ON(ndevs == 0);
-
- return 0;
+ ret = 0;
+out:
+ if (fd != -1)
+ close(fd);
+ return ret;
}
#define isoctal(c) (((c) & ~7) == ''0'')
diff --git a/utils.h b/utils.h
index 8e0252b..885b9c5 100644
--- a/utils.h
+++ b/utils.h
@@ -50,7 +50,7 @@ u64 parse_size(char *s);
int open_file_or_dir(const char *fname);
int get_device_info(int fd, u64 devid,
struct btrfs_ioctl_dev_info_args *di_args);
-int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
+int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret);
int get_label(const char *btrfs_dev);
int set_label(const char *btrfs_dev, const char *label);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen
2013-Mar-12 04:17 UTC
[PATCH 4/4 V2] btrfs-progs: rework get_fs_info to remove side effects
get_fs_info() has been silently switching from a device to a mounted
path as needed; the caller''s filehandle was unexpectedly closed &
reopened outside the caller''s scope. Not so great.
The callers do want "fdmnt" to be the filehandle for the mount point
in all cases, though - the various ioctls act on this (not on an fd
for the device). But switching it in the local scope of get_fs_info
is incorrect; it just so happens that *usually* the fd number is
unchanged.
So - use the new helpers to detect when an argument is a block
device, and open the the mounted path more obviously / explicitly
for ioctl use, storing the filehandle in fdmnt.
Then, in get_fs_info, ignore the fd completely, and use the path on
the argument to determine if the caller wanted to act on just that
device, or on all devices for the filesystem.
Affects those commands which are documented to accept either
a block device or a path:
* btrfs device stats
* btrfs replace start
* btrfs scrub start
* btrfs scrub status
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: don''t call BTRFS_IOC_FS_INFO in the single device case
after we change path/fd to be for the fs mount point.
In the single device case we manually filled in fi_args;
calling this ioctl after switching fd/path to the mount point
overwrites that setup.
diff --git a/cmds-device.c b/cmds-device.c
index 58df6da..41e79d3 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -321,13 +321,14 @@ static int cmd_dev_stats(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
fprintf(stderr, "ERROR: can''t access
''%s''\n", path);
return 12;
}
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for devstats failed: "
"%s\n", strerror(-ret));
diff --git a/cmds-replace.c b/cmds-replace.c
index 10030f6..6397bb5 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -168,7 +168,9 @@ static int cmd_start_replace(int argc, char **argv)
if (check_argc_exact(argc - optind, 3))
usage(cmd_start_replace_usage);
path = argv[optind + 2];
- fdmnt = open_file_or_dir(path);
+
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
fprintf(stderr, "ERROR: can''t access \"%s\":
%s\n",
path, strerror(errno));
@@ -215,7 +217,7 @@ static int cmd_start_replace(int argc, char **argv)
}
start_args.start.srcdevid = (__u64)atoi(srcdev);
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for devstats failed: "
"%s\n", strerror(-ret));
diff --git a/cmds-scrub.c b/cmds-scrub.c
index e5fccc7..52264f1 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1101,13 +1101,14 @@ static int scrub_start(int argc, char **argv, int
resume)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
ERR(!do_quiet, "ERROR: can''t access
''%s''\n", path);
return 12;
}
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1558,13 +1559,14 @@ static int cmd_scrub_status(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
+ fdmnt = open_path_or_dev_mnt(path);
+
if (fdmnt < 0) {
fprintf(stderr, "ERROR: can''t access to
''%s''\n", path);
return 12;
}
- ret = get_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = get_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
diff --git a/utils.c b/utils.c
index 4bf457f..c756e23 100644
--- a/utils.c
+++ b/utils.c
@@ -717,7 +717,7 @@ int open_path_or_dev_mnt(const char *path)
errno = EINVAL;
return -1;
}
- fdmnt = open(mp, O_RDWR);
+ fdmnt = open_file_or_dir(mp);
} else
fdmnt = open_file_or_dir(path);
@@ -1544,9 +1544,20 @@ int get_device_info(int fd, u64 devid,
return ret ? -errno : 0;
}
-int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
+/*
+ * For a given path, fill in the ioctl fs_ and info_ args.
+ * If the path is a btrfs mountpoint, fill info for all devices.
+ * If the path is a btrfs device, fill in only that device.
+ *
+ * The path provided must be either on a mounted btrfs fs,
+ * or be a mounted btrfs device.
+ *
+ * Returns 0 on success, or a negative errno.
+ */
+int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret)
{
+ int fd = -1;
int ret = 0;
int ndevs = 0;
int i = 1;
@@ -1556,33 +1567,56 @@ int get_fs_info(int fd, char *path, struct
btrfs_ioctl_fs_info_args *fi_args,
memset(fi_args, 0, sizeof(*fi_args));
- ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
- if (ret && (errno == EINVAL || errno == ENOTTY)) {
- /* path is not a mounted btrfs. Try if it''s a device */
+ if (is_block_device(path)) {
+ /* Ensure it''s mounted, then set path to the mountpoint */
+ fd = open(path, O_RDONLY);
+ if (fd < 0) {
+ ret = -errno;
+ fprintf(stderr, "Couldn''t open %s: %s\n",
+ path, strerror(errno));
+ goto out;
+ }
ret = check_mounted_where(fd, path, mp, sizeof(mp),
&fs_devices_mnt);
- if (!ret)
- return -EINVAL;
+ if (!ret) {
+ ret = -EINVAL;
+ goto out;
+ }
if (ret < 0)
- return ret;
+ goto out;
+ path = mp;
+ /* Only fill in this one device */
fi_args->num_devices = 1;
fi_args->max_id = fs_devices_mnt->latest_devid;
i = fs_devices_mnt->latest_devid;
memcpy(fi_args->fsid, fs_devices_mnt->fsid, BTRFS_FSID_SIZE);
close(fd);
- fd = open_file_or_dir(mp);
- if (fd < 0)
- return -errno;
- } else if (ret) {
- return -errno;
+ }
+
+ /* at this point path must not be for a block device */
+ fd = open_file_or_dir(path);
+ if (fd < 0) {
+ ret = -errno;
+ goto out;
+ }
+
+ /* fill in fi_args if not just a single device */
+ if (fi_args->num_devices != 1) {
+ ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
+ if (ret < 0) {
+ ret = -errno;
+ goto out;
+ }
}
if (!fi_args->num_devices)
- return 0;
+ goto out;
di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
- if (!di_args)
- return -errno;
+ if (!di_args) {
+ ret = -errno;
+ goto out;
+ }
for (; i <= fi_args->max_id; ++i) {
BUG_ON(ndevs >= fi_args->num_devices);
@@ -1590,13 +1624,16 @@ int get_fs_info(int fd, char *path, struct
btrfs_ioctl_fs_info_args *fi_args,
if (ret == -ENODEV)
continue;
if (ret)
- return ret;
+ goto out;
ndevs++;
}
BUG_ON(ndevs == 0);
-
- return 0;
+ ret = 0;
+out:
+ if (fd != -1)
+ close(fd);
+ return ret;
}
#define isoctal(c) (((c) & ~7) == ''0'')
diff --git a/utils.h b/utils.h
index 8e0252b..885b9c5 100644
--- a/utils.h
+++ b/utils.h
@@ -50,7 +50,7 @@ u64 parse_size(char *s);
int open_file_or_dir(const char *fname);
int get_device_info(int fd, u64 devid,
struct btrfs_ioctl_dev_info_args *di_args);
-int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args,
+int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret);
int get_label(const char *btrfs_dev);
int set_label(const char *btrfs_dev, const char *label);
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Sterba
2013-Mar-12 16:40 UTC
Re: [PATCH 4/4 V2] btrfs-progs: rework get_fs_info to remove side effects
On Mon, Mar 11, 2013 at 11:17:40PM -0500, Eric Sandeen wrote:> * btrfs scrub start > * btrfs scrub statusI did a quick test here: $ mkfs.btrfs -d raid10 -m raid10 /dev/sda[5678] $ mount /dev/sda5 /mnt $ <fill with files> & $ btrfs scrub start /dev/sda5 (ok) $ btrfs scrub status /dev/sda5 (ok) $ btrfs scrub status /mnt (ok) $ btrfs scrub cancel /mnt ^C^C^C^C^C^C^C^C<hang> (exitted after a few minutes) Name: btrfs State: D (disk sleep) Pid: 24214 [<ffffffffa008f625>] btrfs_scrub_cancel+0xc5/0x130 [btrfs] [<ffffffffa006ab40>] btrfs_ioctl+0x13b0/0x1d90 [btrfs] [<ffffffff81172ee6>] do_vfs_ioctl+0x96/0x560 [<ffffffff81173407>] sys_ioctl+0x57/0x90 [<ffffffff819698d9>] system_call_fastpath+0x16/0x1b [<ffffffffffffffff>] 0xffffffffffffffff (gdb) l *(btrfs_scrub_cancel+0xc5) 0x8f625 is in btrfs_scrub_cancel (fs/btrfs/scrub.c:2983). 2978 } 2979 2980 atomic_inc(&fs_info->scrub_cancel_req); 2981 while (atomic_read(&fs_info->scrubs_running)) { 2982 mutex_unlock(&fs_info->scrub_lock); 2983 wait_event(fs_info->scrub_pause_wait, ^^^^ 2984 atomic_read(&fs_info->scrubs_running) == 0); 2985 mutex_lock(&fs_info->scrub_lock); 2986 } 2987 atomic_dec(&fs_info->scrub_cancel_req); I don''t know yet if this happens without this changes, will check next. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Sterba
2013-Mar-12 17:30 UTC
Re: [PATCH 4/4 V2] btrfs-progs: rework get_fs_info to remove side effects
On Tue, Mar 12, 2013 at 05:40:27PM +0100, David Sterba wrote:> I don''t know yet if this happens without this changes, will check next.Happens as well, not a bug in your patches. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html