-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 9/13/14, 3:31 AM, Anand Jain wrote:
>
>
> 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 ?
My initial reaction is to avoid playing naming names within the
kernel. But since it's device mapper-specific, it might not be too
messy to do that. In addition to the patch to the progs, we're also
carrying a patch to systemd since it has it's own little ioctl wrapper
to do the scanning. It needed to be fixed there as well.
- -Jeff
> 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/<name>.
>>
>> 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 <je...@suse.com> --- 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/<name> + * 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 <name>\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);
>>
> -- To unsubscribe from this list: send the line "unsubscribe
> linux-btrfs" in the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
iQIcBAEBAgAGBQJUFhr9AAoJEB57S2MheeWyXzgQAMKR0b29uU0kyYbalOxjpVkB
yAvR5kFItcEgRSfUUyqJqL+atJZ2PrzU4fQsz6i25cOBfQC5EdZYO/QENXe7HleU
oCECYucZC14qTLwswhpMeCtyMrX98sjhQxZ6NPSmLZWi/Btc2QDdwwbij4B3KRnQ
lE/AWmtT/J0fqlmjr18ETdTjTCZVyxUpN809hZpHAwQAiDZhXqbTUveaFu3DpTyn
vDdM5LZ2oGPjS/3WgoNiFy36R1cnxIh3k/+aM/afU20T//dOn8Cdrh+k868hv9fV
f3YfkyoHzwBCXp8VaOZytkOy7HCLbTEpnnB7MbvrNo1ukOlrFZRVRMSUrco2QD6x
4UYfgBj/zuzxIUZotEnpAmE8ppbMBylqjx+dyIwviDCU0Huw9mEnraBf3Yunuf3k
za92pv/SYJnEFRl2w5ULhtR6GJ3RM8TQJOtWRJjPLkS3L30xvVOQZ1zXoazi/4yX
qp7bKKN/hzC8mPvp8aoCW5c+F652e4pmUplbR4RDaNCD7ipv60XfQBVYOJOGh2YX
+OCYERzFs4th3uPlKNDSfsLHwGY36bPC3mPdiaJW144MX1vHL5YKJiEKcnk5QZNY
BwWidSvdylitAmZeKeCRTOJJh/b+Q7/K72RdWSl6g5qipa/00Gc97Sl/jNOyZ7K6
URPW75jcC/KtA2gIArtL
=fAyc
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
