From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:18832 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751664AbaIMH1P (ORCPT ); Sat, 13 Sep 2014 03:27:15 -0400 Message-ID: <5413F2D0.3020006@oracle.com> Date: Sat, 13 Sep 2014 15:31:28 +0800 From: Anand Jain MIME-Version: 1.0 To: Jeff Mahoney CC: linux-btrfs , David Sterba Subject: Re: [PATCH] btrfs-progs: canonicalize pathnames for device commands References: <538F84DF.3090300@suse.com> In-Reply-To: <538F84DF.3090300@suse.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: Jeff, Nice patch. However its better if we do this in the btrfs kernel function btrfs_scan_one_device(). Since the non-canonicalize path can still sneak through the btrfs specific mount option "device=". Any comments ? Thanks, Anand On 06/05/2014 04:43 AM, Jeff Mahoney wrote: > mount(8) will canonicalize pathnames before passing them to the kernel. > Links to e.g. /dev/sda will be resolved to /dev/sda. Links to /dev/dm-# > will be resolved using the name of the device mapper table to > /dev/mapper/. > > Btrfs will use whatever name the user passes to it, regardless of whether > it is canonical or not. That means that if a 'btrfs device ready' is > issued on any device node pointing to the original device, it will adopt > the new name instead of the name that was used during mount. > > Mounting using /dev/sdb2 will result in df: > /dev/sdb2 209715200 39328 207577088 1% /mnt > > # ls -la /dev/whatever-i-like > lrwxrwxrwx 1 root root 4 Jun 4 13:36 /dev/whatever-i-like -> sdb2 > # btrfs dev ready /dev/whatever-i-like > # df /mnt > /dev/whatever-i-like 209715200 39328 207577088 1% /mnt > > Likewise, mounting with /dev/mapper/whatever and using /dev/dm-0 with a > btrfs device command results in df showing /dev/dm-0. This can happen with > multipath devices with friendly names enabled and doing something like > 'partprobe' which (at least with our version) ends up issuing a 'change' > uevent on the sysfs node. That *always* uses the dm-# name, and we get > confused users. > > This patch does the same canonicalization of the paths that mount does > so that we don't end up having inconsistent names reported by ->show_devices > later. > > Signed-off-by: Jeff Mahoney > --- > cmds-device.c | 60 ++++++++++++++++++++++++++++++++++++++++++++------------- > cmds-replace.c | 13 ++++++++++-- > utils.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > utils.h | 2 + > 4 files changed, 117 insertions(+), 15 deletions(-) > > --- a/cmds-device.c > +++ b/cmds-device.c > @@ -95,6 +95,7 @@ static int cmd_add_dev(int argc, char ** > int devfd, res; > u64 dev_block_count = 0; > int mixed = 0; > + char *path; > > res = test_dev_for_mkfs(argv[i], force, estr); > if (res) { > @@ -118,15 +119,24 @@ static int cmd_add_dev(int argc, char ** > goto error_out; > } > > - strncpy_null(ioctl_args.name, argv[i]); > + path = canonicalize_path(argv[i]); > + if (!path) { > + fprintf(stderr, > + "ERROR: Could not canonicalize pathname '%s': %s\n", > + argv[i], strerror(errno)); > + ret++; > + goto error_out; > + } > + > + strncpy_null(ioctl_args.name, path); > res = ioctl(fdmnt, BTRFS_IOC_ADD_DEV, &ioctl_args); > e = errno; > - if(res<0){ > + if (res < 0) { > fprintf(stderr, "ERROR: error adding the device '%s' - %s\n", > - argv[i], strerror(e)); > + path, strerror(e)); > ret++; > } > - > + free(path); > } > > error_out: > @@ -242,6 +252,7 @@ static int cmd_scan_dev(int argc, char * > > for( i = devstart ; i < argc ; i++ ){ > struct btrfs_ioctl_vol_args args; > + char *path; > > if (!is_block_device(argv[i])) { > fprintf(stderr, > @@ -249,9 +260,17 @@ static int cmd_scan_dev(int argc, char * > ret = 1; > goto close_out; > } > - printf("Scanning for Btrfs filesystems in '%s'\n", argv[i]); > + path = canonicalize_path(argv[i]); > + if (!path) { > + fprintf(stderr, > + "ERROR: Could not canonicalize path '%s': %s\n", > + argv[i], strerror(errno)); > + ret = 1; > + goto close_out; > + } > + printf("Scanning for Btrfs filesystems in '%s'\n", path); > > - strncpy_null(args.name, argv[i]); > + strncpy_null(args.name, path); > /* > * FIXME: which are the error code returned by this ioctl ? > * it seems that is impossible to understand if there no is > @@ -262,9 +281,11 @@ static int cmd_scan_dev(int argc, char * > > if( ret < 0 ){ > fprintf(stderr, "ERROR: unable to scan the device '%s' - %s\n", > - argv[i], strerror(e)); > + path, strerror(e)); > + free(path); > goto close_out; > } > + free(path); > } > > close_out: > @@ -284,6 +305,7 @@ static int cmd_ready_dev(int argc, char > struct btrfs_ioctl_vol_args args; > int fd; > int ret; > + char *path; > > if (check_argc_min(argc, 2)) > usage(cmd_ready_dev_usage); > @@ -293,22 +315,34 @@ static int cmd_ready_dev(int argc, char > perror("failed to open /dev/btrfs-control"); > return 1; > } > - if (!is_block_device(argv[1])) { > + > + path = canonicalize_path(argv[argc - 1]); > + if (!path) { > fprintf(stderr, > - "ERROR: %s is not a block device\n", argv[1]); > - close(fd); > - return 1; > + "ERROR: Could not canonicalize pathname '%s': %s\n", > + argv[argc - 1], strerror(errno)); > + ret = 1; > + goto out; > } > > - strncpy(args.name, argv[argc - 1], BTRFS_PATH_NAME_MAX); > + if (!is_block_device(path)) { > + fprintf(stderr, > + "ERROR: %s is not a block device\n", path); > + ret = 1; > + goto out; > + } > + > + strncpy(args.name, path, BTRFS_PATH_NAME_MAX); > ret = ioctl(fd, BTRFS_IOC_DEVICES_READY, &args); > if (ret < 0) { > fprintf(stderr, "ERROR: unable to determine if the device '%s'" > - " is ready for mounting - %s\n", argv[argc - 1], > + " is ready for mounting - %s\n", path, > strerror(errno)); > ret = 1; > } > > +out: > + free(path); > close(fd); > return ret; > } > --- a/cmds-replace.c > +++ b/cmds-replace.c > @@ -134,7 +134,7 @@ static int cmd_start_replace(int argc, c > int fddstdev = -1; > char *path; > char *srcdev; > - char *dstdev; > + char *dstdev = NULL; > int avoid_reading_from_srcdev = 0; > int force_using_targetdev = 0; > struct stat st; > @@ -204,7 +204,12 @@ static int cmd_start_replace(int argc, c > } > > srcdev = argv[optind]; > - dstdev = argv[optind + 1]; > + dstdev = canonicalize_path(argv[optind + 1]); > + if (!dstdev) { > + fprintf(stderr, > + "ERROR: Could not canonicalize path '%s': %s\n", > + argv[optind + 1], strerror(errno)); > + } > > if (is_numerical(srcdev)) { > struct btrfs_ioctl_fs_info_args fi_args; > @@ -278,6 +283,8 @@ static int cmd_start_replace(int argc, c > > close(fddstdev); > fddstdev = -1; > + free(dstdev); > + dstdev = NULL; > > dev_replace_handle_sigint(fdmnt); > if (!do_not_background) { > @@ -312,6 +319,8 @@ static int cmd_start_replace(int argc, c > return 0; > > leave_with_error: > + if (dstdev) > + free(dstdev); > if (fdmnt != -1) > close(fdmnt); > if (fdsrcdev != -1) > --- a/utils.c > +++ b/utils.c > @@ -987,6 +987,63 @@ static int blk_file_in_dev_list(struct b > } > > /* > + * Resolve a pathname to a device mapper node to /dev/mapper/ > + * Returns NULL on invalid input or malloc failure; Other failures > + * will be handled by the caller using the input pathame. > + */ > +char *canonicalize_dm_name(const char *ptname) > +{ > + FILE *f; > + size_t sz; > + char path[256], name[256], *res = NULL; > + > + if (!ptname || !*ptname) > + return NULL; > + > + snprintf(path, sizeof(path), "/sys/block/%s/dm/name", ptname); > + if (!(f = fopen(path, "r"))) > + return NULL; > + > + /* read \n from sysfs */ > + if (fgets(name, sizeof(name), f) && (sz = strlen(name)) > 1) { > + name[sz - 1] = '\0'; > + snprintf(path, sizeof(path), "/dev/mapper/%s", name); > + > + if (access(path, F_OK) == 0) > + res = strdup(path); > + } > + fclose(f); > + return res; > +} > + > +/* > + * Resolve a pathname to a canonical device node, e.g. /dev/sda1 or > + * to a device mapper pathname. > + * Returns NULL on invalid input or malloc failure; Other failures > + * will be handled by the caller using the input pathame. > + */ > +char *canonicalize_path(const char *path) > +{ > + char *canonical, *p; > + > + if (!path || !*path) > + return NULL; > + > + canonical = realpath(path, NULL); > + if (!canonical) > + return strdup(path); > + p = strrchr(canonical, '/'); > + if (p && strncmp(p, "/dm-", 4) == 0 && isdigit(*(p + 4))) { > + char *dm = canonicalize_dm_name(p + 1); > + if (dm) { > + free(canonical); > + return dm; > + } > + } > + return canonical; > +} > + > +/* > * returns 1 if the device was mounted, < 0 on error or 0 if everything > * is safe to continue. > */ > --- a/utils.h > +++ b/utils.h > @@ -61,6 +61,8 @@ int btrfs_add_to_fsid(struct btrfs_trans > int btrfs_scan_for_fsid(int run_ioctls); > void btrfs_register_one_device(char *fname); > int btrfs_scan_one_dir(char *dirname, int run_ioctl); > +char *canonicalize_dm_name(const char *ptname); > +char *canonicalize_path(const char *path); > int check_mounted(const char *devicename); > int check_mounted_where(int fd, const char *file, char *where, int size, > struct btrfs_fs_devices **fs_devices_mnt); >