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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
