[PATCH 0/5] btrfs-progs: device_get_partition_size*() enhancement and cleanup

[PATCH 0/5] btrfs-progs: device_get_partition_size*() enhancement and cleanup

[Qu Wenruo]

Inspired by recent fixes from Zotan to device_get_partition_size_sysfs(),
it turns out the idea of falling back to use sysfs is not sound in the first
place.

The original issue is unprivileged user can cause `btrfs device usage`
to output incorrect values:

  WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
  /dev/mapper/test-scratch1, ID: 1
     Device size:            20.00MiB <<
     Device slack:           16.00EiB <<
     Unallocated:                 N/A

It turns out that, the "unprivileged" part means, the user doesn't have
read access to the raw device.

It's common that a lot of distros have the following mode for raw block
devices:

  brw-rw---- 1 root disk 254, 32 Aug  2 13:35 /dev/vdc

So unprivileged users can not do read access to block devices thus
failed to grab the block device size, and this is normally considered as
a safety method.

But if the end user is inside disk group, everything works fine:

  WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
  /dev/mapper/test-scratch1, ID: 1
     Device size:            10.00GiB
     Device slack:              0.00B
     Unallocated:                 N/A

And the original bug is that the sysfs interface only gives the device
size in block unit, and block size can be hard to fetch (partition and
dm have different path to grab the block size).


This makes me wonder, why we need the sysfs interface in the first
place.
If the unprivileged user has no access to a block device, we should
not workaround it, but give a proper error/warning message.

So this series address the root problem, no proper error code for
device_get_partition_size*() helpers.
With proper error handling and messages, we shouldn't even need to use
sysfs to workaround the problem.

With the sysfs fallback removed, a completely unprivileged user will get
the following result:

  WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
  WARNING: failed to get device size for devid 1: Permission denied
  /dev/mapper/test-scratch1, ID: 1
     Device size:               0.00B
     Device slack:              0.00B
     Unallocated:                 N/A

Which should give enough for the end user without weird output at all.

Qu Wenruo (5):
  btrfs-progs: remove the unnecessary device_get_partition_size() call
  btrfs-progs: add error handling for device_get_partition_size()
  btrfs-progs: add error handling for
    device_get_partition_size_fd_stat()
  btrfs-progs: remove device_get_partition_size_fd()
  btrfs-progs: remove device_get_partition_size_sysfs()

 check/main.c            |  8 +++-
 check/mode-lowmem.c     |  8 +++-
 cmds/filesystem-usage.c | 11 ++++--
 cmds/replace.c          | 14 ++++++-
 common/device-utils.c   | 87 ++++++++++-------------------------------
 common/device-utils.h   |  5 +--
 image/common.c          |  7 +++-
 kernel-shared/volumes.c |  9 ++++-
 kernel-shared/zoned.c   | 18 ++++++---
 mkfs/common.c           | 16 ++++++--
 mkfs/main.c             | 12 ++++--
 11 files changed, 103 insertions(+), 92 deletions(-)

--
2.50.1

[PATCH 1/5] btrfs-progs: remove the unnecessary device_get_partition_size() call

[Qu Wenruo]

Inside function _cmd_filesystem_usage_tabular(), there is a call of
device_get_partition_size() to calculate @unused.

However immediately after that call, @unused is updated using
devinfo->size, so that the previous call of device_get_partition_size()
is completely useless.

Just remove the unnecessary call.

And since we're here, also remove a dead newline in that loop at
variable declaration.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem-usage.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index 473479a1d72d..f812af13482e 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -959,7 +959,6 @@ static void _cmd_filesystem_usage_tabular(unsigned unit_mode,
 		int k;
 		const char *p;
 		const struct device_info *devinfo = devinfos->data[i];
-
 		u64  total_allocated = 0, unused;
 
 		p = strrchr(devinfo->path, '/');
@@ -1000,7 +999,6 @@ static void _cmd_filesystem_usage_tabular(unsigned unit_mode,
 			col++;
 		}
 
-		unused = device_get_partition_size(devinfo->path) - total_allocated;
 		unused = devinfo->size - total_allocated;
 
 		table_printf(matrix, unallocated_col, vhdr_skip + i, ">%s",
-- 
2.50.1

[PATCH 2/5] btrfs-progs: add error handling for device_get_partition_size()

[Qu Wenruo]

The function device_get_partition_size() has all kinds of error paths,
but it has no way to return error other than returning 0.

This is not helpful for end users to know what's going wrong.

Change that function to have a dedicated return value for error
handling, and pass a u64 pointer for the result.

For most callers they are able to exit gracefully with a proper error
message.

But for load_device_info(), we allow a failed size probing to continue
without 0 size, just as the old code.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem-usage.c |  9 +++++++--
 cmds/replace.c          | 14 ++++++++++++--
 common/device-utils.c   | 29 +++++++++++++++--------------
 common/device-utils.h   |  2 +-
 4 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index f812af13482e..e367bffc3a01 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -820,8 +820,13 @@ static int load_device_info(int fd, struct array *devinfos)
 			strcpy(info->path, "missing");
 		} else {
 			strcpy(info->path, (char *)dev_info.path);
-			info->device_size =
-				device_get_partition_size((const char *)dev_info.path);
+			ret = device_get_partition_size((const char *)dev_info.path,
+							&info->device_size);
+			if (ret < 0) {
+				errno = -ret;
+				warning("failed to get device size for devid %llu: %m", dev_info.devid);
+				info->device_size = 0;
+			}
 		}
 		info->size = dev_info.total_bytes;
 		ndevs++;
diff --git a/cmds/replace.c b/cmds/replace.c
index 887c3251a725..d5b0b0e02ce1 100644
--- a/cmds/replace.c
+++ b/cmds/replace.c
@@ -269,7 +269,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 		strncpy_null((char *)start_args.start.srcdev_name, srcdev,
 			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
 		start_args.start.srcdevid = 0;
-		srcdev_size = device_get_partition_size(srcdev);
+		ret = device_get_partition_size(srcdev, &srcdev_size);
+		if (ret < 0) {
+			errno = -ret;
+			error("failed to get device size for %s: %m", srcdev);
+			goto leave_with_error;
+		}
 	} else {
 		error("source device must be a block device or a devid");
 		goto leave_with_error;
@@ -279,7 +284,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 	if (ret)
 		goto leave_with_error;
 
-	dstdev_size = device_get_partition_size(dstdev);
+	ret = device_get_partition_size(dstdev, &dstdev_size);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to get device size for %s: %m", dstdev);
+		goto leave_with_error;
+	}
 	if (srcdev_size > dstdev_size) {
 		error("target device smaller than source device (required %llu bytes)",
 			srcdev_size);
diff --git a/common/device-utils.c b/common/device-utils.c
index 783d79555446..6d89d16b029d 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -342,7 +342,7 @@ u64 device_get_partition_size_fd(int fd)
 	return result;
 }
 
-static u64 device_get_partition_size_sysfs(const char *dev)
+static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
 {
 	int ret;
 	char path[PATH_MAX] = {};
@@ -354,45 +354,46 @@ static u64 device_get_partition_size_sysfs(const char *dev)
 
 	name = realpath(dev, path);
 	if (!name)
-		return 0;
+		return -errno;
 	name = path_basename(path);
 
 	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
 	if (ret < 0)
-		return 0;
+		return ret;
 	sysfd = open(sysfs, O_RDONLY);
 	if (sysfd < 0)
-		return 0;
+		return -errno;
 	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
 	if (ret < 0) {
 		close(sysfd);
-		return 0;
+		return ret;
 	}
 	errno = 0;
 	size = strtoull(sizebuf, NULL, 10);
 	if (size == ULLONG_MAX && errno == ERANGE) {
 		close(sysfd);
-		return 0;
+		return -errno;
 	}
 	close(sysfd);
-	return size;
+	*result_ret = size;
+	return 0;
 }
 
-u64 device_get_partition_size(const char *dev)
+int device_get_partition_size(const char *dev, u64 *result_ret)
 {
-	u64 result;
 	int fd = open(dev, O_RDONLY);
 
 	if (fd < 0)
-		return device_get_partition_size_sysfs(dev);
+		return device_get_partition_size_sysfs(dev, result_ret);
+
+	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
+		int ret = -errno;
 
-	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
 		close(fd);
-		return 0;
+		return ret;
 	}
 	close(fd);
-
-	return result;
+	return 0;
 }
 
 /*
diff --git a/common/device-utils.h b/common/device-utils.h
index cef9405f3a9a..bf04eb0f2fdd 100644
--- a/common/device-utils.h
+++ b/common/device-utils.h
@@ -42,7 +42,7 @@ enum {
  */
 int device_discard_blocks(int fd, u64 start, u64 len);
 int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
-u64 device_get_partition_size(const char *dev);
+int device_get_partition_size(const char *dev, u64 *result_ret);
 u64 device_get_partition_size_fd(int fd);
 u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
 int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
-- 
2.50.1

[PATCH 3/5] btrfs-progs: add error handling for device_get_partition_size_fd_stat()

[Qu Wenruo]

The function device_get_partition_size_fd_state() has two different
error paths:

- The target file is not a regular nor block file
  This should be the common one.

- ioctl failed
  This should be very rare.

But it has no way to return error other than returning 0.
This is not helpful for end users to know what's going wrong.

Change that function to have a dedicated return value for error
handling, and pass a u64 pointer for the result.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c            |  8 +++++++-
 check/mode-lowmem.c     |  8 +++++++-
 common/device-utils.c   | 24 ++++++++++++++----------
 common/device-utils.h   |  2 +-
 image/common.c          |  7 ++++++-
 kernel-shared/volumes.c |  9 +++++++--
 kernel-shared/zoned.c   | 18 +++++++++++++-----
 mkfs/common.c           | 16 ++++++++++++----
 mkfs/main.c             | 12 +++++++++---
 9 files changed, 76 insertions(+), 28 deletions(-)

diff --git a/check/main.c b/check/main.c
index 84b6de597072..8bc1a164e05d 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5447,7 +5447,13 @@ static int process_device_item(struct rb_root *dev_cache,
 				device->devid);
 			goto skip;
 		}
-		block_dev_size = device_get_partition_size_fd_stat(device->fd, &st);
+		ret = device_get_partition_size_fd_stat(device->fd, &st, &block_dev_size);
+		if (ret < 0) {
+			warning(
+	"failed to get device size for %s, skipping its block device size check: %m",
+				device->name);
+			goto skip;
+		}
 		if (block_dev_size < rec->total_byte) {
 			error(
 "block device size is smaller than total_bytes in device item, has %llu expect >= %llu",
diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index e05bb0da1709..9aec7eab6e90 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4822,7 +4822,13 @@ next:
 			dev->devid);
 		return 0;
 	}
-	block_dev_size = device_get_partition_size_fd_stat(dev->fd, &st);
+	ret = device_get_partition_size_fd_stat(dev->fd, &st, &block_dev_size);
+	if (ret < 0) {
+		warning(
+	"failed to get device size for %s, skipping its block device size check: %m",
+			dev->name);
+		return 0;
+	}
 	if (block_dev_size < total_bytes) {
 		error(
 "block device size is smaller than total_bytes in device item, has %llu expect >= %llu",
diff --git a/common/device-utils.c b/common/device-utils.c
index 6d89d16b029d..2912375a4d21 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -236,9 +236,10 @@ int btrfs_prepare_device(int fd, const char *file, u64 *byte_count_ret,
 		return 1;
 	}
 
-	byte_count = device_get_partition_size_fd_stat(fd, &st);
-	if (byte_count == 0) {
-		error("unable to determine size of %s", file);
+	ret = device_get_partition_size_fd_stat(fd, &st, &byte_count);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to get device size for %s: %m", file);
 		return 1;
 	}
 	if (max_byte_count)
@@ -315,17 +316,20 @@ err:
 	return 1;
 }
 
-u64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
+int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result_ret)
 {
-	u64 size;
+	int ret;
 
-	if (S_ISREG(st->st_mode))
-		return st->st_size;
-	if (!S_ISBLK(st->st_mode))
+	if (S_ISREG(st->st_mode)) {
+		*result_ret = st->st_size;
 		return 0;
-	if (ioctl(fd, BLKGETSIZE64, &size) >= 0)
-		return size;
+	}
+	if (!S_ISBLK(st->st_mode))
+		return -EINVAL;
 
+	ret = ioctl(fd, BLKGETSIZE64, result_ret);
+	if (ret < 0)
+		return -errno;
 	return 0;
 }
 
diff --git a/common/device-utils.h b/common/device-utils.h
index bf04eb0f2fdd..c55b6944693a 100644
--- a/common/device-utils.h
+++ b/common/device-utils.h
@@ -44,7 +44,7 @@ int device_discard_blocks(int fd, u64 start, u64 len);
 int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
 int device_get_partition_size(const char *dev, u64 *result_ret);
 u64 device_get_partition_size_fd(int fd);
-u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
+int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result_ret);
 int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
 u64 device_get_zone_unusable(int fd, u64 flags);
 u64 device_get_zone_size(int fd, const char *name);
diff --git a/image/common.c b/image/common.c
index 3faf803f91e5..74ad7fd426bb 100644
--- a/image/common.c
+++ b/image/common.c
@@ -118,7 +118,12 @@ void write_backup_supers(int fd, u8 *buf)
 		return;
 	}
 
-	size = device_get_partition_size_fd_stat(fd, &st);
+	ret = device_get_partition_size_fd_stat(fd, &st, &size);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to get device size: %m");
+		return;
+	}
 
 	for (i = 1; i < BTRFS_SUPER_MIRROR_MAX; i++) {
 		bytenr = btrfs_sb_offset(i);
diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index be01bdb4d3f6..0608933ebc9a 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -3096,8 +3096,13 @@ static int btrfs_fix_block_device_size(struct btrfs_fs_info *fs_info,
 		return -errno;
 	}
 
-	block_dev_size = round_down(device_get_partition_size_fd_stat(device->fd, &st),
-				    fs_info->sectorsize);
+	ret = device_get_partition_size_fd_stat(device->fd, &st, &block_dev_size);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to get device size for %s: %m", device->name);
+		return ret;
+	}
+	block_dev_size = round_down(block_dev_size, fs_info->sectorsize);
 
 	/*
 	 * Total_bytes in device item is no larger than the device block size,
diff --git a/kernel-shared/zoned.c b/kernel-shared/zoned.c
index d96311af70b2..f88f4fbd3465 100644
--- a/kernel-shared/zoned.c
+++ b/kernel-shared/zoned.c
@@ -166,7 +166,8 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
 {
 	const sector_t zone_sectors = emulated_zone_size >> SECTOR_SHIFT;
 	struct stat st;
-	sector_t bdev_size;
+	u64 bdev_size;
+	sector_t bdev_nr_sectors;
 	unsigned int i;
 	int ret;
 
@@ -176,7 +177,13 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
 		return -EIO;
 	}
 
-	bdev_size = device_get_partition_size_fd_stat(fd, &st) >> SECTOR_SHIFT;
+	ret = device_get_partition_size_fd_stat(fd, &st, &bdev_size);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to get device size for %s: %m", file);
+		return ret;
+	}
+	bdev_nr_sectors = bdev_size >> SECTOR_SHIFT;
 
 	pos >>= SECTOR_SHIFT;
 	for (i = 0; i < nr_zones; i++) {
@@ -187,7 +194,7 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
 		zones[i].type = BLK_ZONE_TYPE_CONVENTIONAL;
 		zones[i].cond = BLK_ZONE_COND_NOT_WP;
 
-		if (zones[i].wp >= bdev_size) {
+		if (zones[i].wp >= bdev_nr_sectors) {
 			i++;
 			break;
 		}
@@ -325,8 +332,9 @@ static int report_zones(int fd, const char *file,
 		return -EIO;
 	}
 
-	device_size = device_get_partition_size_fd_stat(fd, &st);
-	if (device_size == 0) {
+	ret = device_get_partition_size_fd_stat(fd, &st, &device_size);
+	if (ret < 0) {
+		errno = -ret;
 		error("zoned: failed to read size of %s: %m", file);
 		exit(1);
 	}
diff --git a/mkfs/common.c b/mkfs/common.c
index d7a1dc5867eb..f146f4a414a6 100644
--- a/mkfs/common.c
+++ b/mkfs/common.c
@@ -1173,6 +1173,7 @@ int is_vol_small(const char *file)
 	int e;
 	struct stat st;
 	u64 size;
+	int ret;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
@@ -1182,10 +1183,10 @@ int is_vol_small(const char *file)
 		close(fd);
 		return e;
 	}
-	size = device_get_partition_size_fd_stat(fd, &st);
-	if (size == 0) {
+	ret = device_get_partition_size_fd_stat(fd, &st, &size);
+	if (ret < 0) {
 		close(fd);
-		return -1;
+		return ret;
 	}
 	if (size < BTRFS_MKFS_SMALL_VOLUME_SIZE) {
 		close(fd);
@@ -1200,6 +1201,8 @@ int test_minimum_size(const char *file, u64 min_dev_size)
 {
 	int fd;
 	struct stat statbuf;
+	u64 device_size;
+	int ret;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
@@ -1208,7 +1211,12 @@ int test_minimum_size(const char *file, u64 min_dev_size)
 		close(fd);
 		return -errno;
 	}
-	if (device_get_partition_size_fd_stat(fd, &statbuf) < min_dev_size) {
+	ret = device_get_partition_size_fd_stat(fd, &statbuf, &device_size);
+	if (ret < 0) {
+		close(fd);
+		return ret;
+	}
+	if (device_size < min_dev_size) {
 		close(fd);
 		return 1;
 	}
diff --git a/mkfs/main.c b/mkfs/main.c
index c3529044a836..546cc74735a9 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1818,9 +1818,15 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 		 * Block_count not specified, use file/device size first.
 		 * Or we will always use source_dir_size calculated for mkfs.
 		 */
-		if (!byte_count)
-			byte_count = round_down(device_get_partition_size_fd_stat(fd, &statbuf),
-						sectorsize);
+		if (!byte_count) {
+			ret = device_get_partition_size_fd_stat(fd, &statbuf, &byte_count);
+			if (ret < 0) {
+				errno = -ret;
+				error("failed to get device size for %s: %m", file);
+				goto error;
+			}
+			byte_count = round_down(byte_count, sectorsize);
+		}
 		source_dir_size = btrfs_mkfs_size_dir(source_dir, sectorsize,
 				min_dev_size, metadata_profile, data_profile);
 		UASSERT(IS_ALIGNED(source_dir_size, sectorsize));
-- 
2.50.1

[PATCH 4/5] btrfs-progs: remove device_get_partition_size_fd()

[Qu Wenruo]

The last user of the helper is removed in commit 585ac14d1af2
("btrfs-progs: use btrfs_device_size() instead of
device_get_partition_size_fd()"), and that helper is not generic enough
to handle regular files.

So let's just remove it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/device-utils.c | 13 -------------
 common/device-utils.h |  1 -
 2 files changed, 14 deletions(-)

diff --git a/common/device-utils.c b/common/device-utils.c
index 2912375a4d21..b52fbf33a539 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -333,19 +333,6 @@ int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result
 	return 0;
 }
 
-/*
- * Read partition size using the low-level ioctl
- */
-u64 device_get_partition_size_fd(int fd)
-{
-	u64 result;
-
-	if (ioctl(fd, BLKGETSIZE64, &result) < 0)
-		return 0;
-
-	return result;
-}
-
 static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
 {
 	int ret;
diff --git a/common/device-utils.h b/common/device-utils.h
index c55b6944693a..28932aba2859 100644
--- a/common/device-utils.h
+++ b/common/device-utils.h
@@ -43,7 +43,6 @@ enum {
 int device_discard_blocks(int fd, u64 start, u64 len);
 int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
 int device_get_partition_size(const char *dev, u64 *result_ret);
-u64 device_get_partition_size_fd(int fd);
 int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result_ret);
 int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
 u64 device_get_zone_unusable(int fd, u64 flags);
-- 
2.50.1

[PATCH 5/5] btrfs-progs: remove device_get_partition_size_sysfs()

[Qu Wenruo]

The helper is introduced for cases where the unprivileged user failed to
open the target file.

The problem is, when such failure happens, it means the distro's file
mode or ACL is actively preventing unrelated users to access the raw
device.

E.g. on my distro the default block device mode looks like this:

  brw-rw---- 1 root disk 254, 32 Aug  2 13:35 /dev/vdc

This means if an unprivileged end user is not in the disk group, it
should access the raw disk.

Using sysfs as a fallback is more like a workaround, and the workaround
is soon getting out of control.

For example the size is not in byte but in block unit. This is already
causing problem for issue #979.

Furthermore to grab the correct size in bytes, we have to do all kinds
of sysfs probing to handle partitions (to get the block size from parent
node) and dm devices (directly from the current node).

With the recent error handling enhancement, I do not think we should
even bother using the sysfs fallback, the error message should be enough
to inform the end user.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/device-utils.c | 39 +--------------------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

diff --git a/common/device-utils.c b/common/device-utils.c
index b52fbf33a539..7a0a299ccf83 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -333,49 +333,12 @@ int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result
 	return 0;
 }
 
-static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
-{
-	int ret;
-	char path[PATH_MAX] = {};
-	char sysfs[PATH_MAX] = {};
-	char sizebuf[128] = {};
-	const char *name = NULL;
-	int sysfd;
-	unsigned long long size = 0;
-
-	name = realpath(dev, path);
-	if (!name)
-		return -errno;
-	name = path_basename(path);
-
-	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
-	if (ret < 0)
-		return ret;
-	sysfd = open(sysfs, O_RDONLY);
-	if (sysfd < 0)
-		return -errno;
-	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
-	if (ret < 0) {
-		close(sysfd);
-		return ret;
-	}
-	errno = 0;
-	size = strtoull(sizebuf, NULL, 10);
-	if (size == ULLONG_MAX && errno == ERANGE) {
-		close(sysfd);
-		return -errno;
-	}
-	close(sysfd);
-	*result_ret = size;
-	return 0;
-}
-
 int device_get_partition_size(const char *dev, u64 *result_ret)
 {
 	int fd = open(dev, O_RDONLY);
 
 	if (fd < 0)
-		return device_get_partition_size_sysfs(dev, result_ret);
+		return -errno;
 
 	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
 		int ret = -errno;
-- 
2.50.1

Re: [PATCH 2/5] btrfs-progs: add error handling for device_get_partition_size()

[Boris Burkov]

On Sat, Aug 02, 2025 at 04:25:48PM +0930, Qu Wenruo wrote:
> The function device_get_partition_size() has all kinds of error paths,
> but it has no way to return error other than returning 0.
> 
> This is not helpful for end users to know what's going wrong.
> 
> Change that function to have a dedicated return value for error
> handling, and pass a u64 pointer for the result.

All the callers check ret < 0, rather than ret != 0, so any reason not
to simply return the positive value in case of no error? Signed
overflow?

In the kernel, BLK_DEV_MAX_SECTORS is set to LLONG_MAX >> 9, which I
think means we are probably OK?

> 
> For most callers they are able to exit gracefully with a proper error
> message.
> 
> But for load_device_info(), we allow a failed size probing to continue
> without 0 size, just as the old code.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  cmds/filesystem-usage.c |  9 +++++++--
>  cmds/replace.c          | 14 ++++++++++++--
>  common/device-utils.c   | 29 +++++++++++++++--------------
>  common/device-utils.h   |  2 +-
>  4 files changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index f812af13482e..e367bffc3a01 100644
> --- a/cmds/filesystem-usage.c
> +++ b/cmds/filesystem-usage.c
> @@ -820,8 +820,13 @@ static int load_device_info(int fd, struct array *devinfos)
>  			strcpy(info->path, "missing");
>  		} else {
>  			strcpy(info->path, (char *)dev_info.path);
> -			info->device_size =
> -				device_get_partition_size((const char *)dev_info.path);
> +			ret = device_get_partition_size((const char *)dev_info.path,
> +							&info->device_size);
> +			if (ret < 0) {
> +				errno = -ret;
> +				warning("failed to get device size for devid %llu: %m", dev_info.devid);
> +				info->device_size = 0;
> +			}
>  		}
>  		info->size = dev_info.total_bytes;
>  		ndevs++;
> diff --git a/cmds/replace.c b/cmds/replace.c
> index 887c3251a725..d5b0b0e02ce1 100644
> --- a/cmds/replace.c
> +++ b/cmds/replace.c
> @@ -269,7 +269,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>  		strncpy_null((char *)start_args.start.srcdev_name, srcdev,
>  			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
>  		start_args.start.srcdevid = 0;
> -		srcdev_size = device_get_partition_size(srcdev);
> +		ret = device_get_partition_size(srcdev, &srcdev_size);
> +		if (ret < 0) {
> +			errno = -ret;
> +			error("failed to get device size for %s: %m", srcdev);
> +			goto leave_with_error;
> +		}
>  	} else {
>  		error("source device must be a block device or a devid");
>  		goto leave_with_error;
> @@ -279,7 +284,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>  	if (ret)
>  		goto leave_with_error;
>  
> -	dstdev_size = device_get_partition_size(dstdev);
> +	ret = device_get_partition_size(dstdev, &dstdev_size);
> +	if (ret < 0) {
> +		errno = -ret;
> +		error("failed to get device size for %s: %m", dstdev);
> +		goto leave_with_error;
> +	}
>  	if (srcdev_size > dstdev_size) {
>  		error("target device smaller than source device (required %llu bytes)",
>  			srcdev_size);
> diff --git a/common/device-utils.c b/common/device-utils.c
> index 783d79555446..6d89d16b029d 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -342,7 +342,7 @@ u64 device_get_partition_size_fd(int fd)
>  	return result;
>  }
>  
> -static u64 device_get_partition_size_sysfs(const char *dev)
> +static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
>  {
>  	int ret;
>  	char path[PATH_MAX] = {};
> @@ -354,45 +354,46 @@ static u64 device_get_partition_size_sysfs(const char *dev)
>  
>  	name = realpath(dev, path);
>  	if (!name)
> -		return 0;
> +		return -errno;
>  	name = path_basename(path);
>  
>  	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
>  	if (ret < 0)
> -		return 0;
> +		return ret;
>  	sysfd = open(sysfs, O_RDONLY);
>  	if (sysfd < 0)
> -		return 0;
> +		return -errno;
>  	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>  	if (ret < 0) {
>  		close(sysfd);
> -		return 0;
> +		return ret;
>  	}
>  	errno = 0;
>  	size = strtoull(sizebuf, NULL, 10);
>  	if (size == ULLONG_MAX && errno == ERANGE) {
>  		close(sysfd);
> -		return 0;
> +		return -errno;
>  	}
>  	close(sysfd);
> -	return size;
> +	*result_ret = size;
> +	return 0;
>  }
>  
> -u64 device_get_partition_size(const char *dev)
> +int device_get_partition_size(const char *dev, u64 *result_ret)
>  {
> -	u64 result;
>  	int fd = open(dev, O_RDONLY);
>  
>  	if (fd < 0)
> -		return device_get_partition_size_sysfs(dev);
> +		return device_get_partition_size_sysfs(dev, result_ret);
> +
> +	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
> +		int ret = -errno;
>  
> -	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
>  		close(fd);
> -		return 0;
> +		return ret;
>  	}
>  	close(fd);
> -
> -	return result;
> +	return 0;
>  }
>  
>  /*
> diff --git a/common/device-utils.h b/common/device-utils.h
> index cef9405f3a9a..bf04eb0f2fdd 100644
> --- a/common/device-utils.h
> +++ b/common/device-utils.h
> @@ -42,7 +42,7 @@ enum {
>   */
>  int device_discard_blocks(int fd, u64 start, u64 len);
>  int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
> -u64 device_get_partition_size(const char *dev);
> +int device_get_partition_size(const char *dev, u64 *result_ret);
>  u64 device_get_partition_size_fd(int fd);
>  u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
>  int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
> -- 
> 2.50.1
> 

Re: [PATCH 5/5] btrfs-progs: remove device_get_partition_size_sysfs()

[Boris Burkov]

On Sat, Aug 02, 2025 at 04:25:51PM +0930, Qu Wenruo wrote:
> The helper is introduced for cases where the unprivileged user failed to
> open the target file.
> 
> The problem is, when such failure happens, it means the distro's file
> mode or ACL is actively preventing unrelated users to access the raw
> device.
> 
> E.g. on my distro the default block device mode looks like this:
> 
>   brw-rw---- 1 root disk 254, 32 Aug  2 13:35 /dev/vdc
> 
> This means if an unprivileged end user is not in the disk group, it
> should access the raw disk.
> 
> Using sysfs as a fallback is more like a workaround, and the workaround
> is soon getting out of control.
> 
> For example the size is not in byte but in block unit. This is already
> causing problem for issue #979.

shifting by sector size doesn't seem like a crazy thing for us to simply
fix.

> 
> Furthermore to grab the correct size in bytes, we have to do all kinds
> of sysfs probing to handle partitions (to get the block size from parent
> node) and dm devices (directly from the current node).

I don't quite understand this justification, is this another fix we
would have to make or is it what we are already doing?

> 
> With the recent error handling enhancement, I do not think we should
> even bother using the sysfs fallback, the error message should be enough
> to inform the end user.

I think that given we have users reporting being confused by size being
off by a factor of 512, it makes more sense to make them happy by just
shifting rather than throwing the whole thing out.

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  common/device-utils.c | 39 +--------------------------------------
>  1 file changed, 1 insertion(+), 38 deletions(-)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index b52fbf33a539..7a0a299ccf83 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -333,49 +333,12 @@ int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result
>  	return 0;
>  }
>  
> -static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
> -{
> -	int ret;
> -	char path[PATH_MAX] = {};
> -	char sysfs[PATH_MAX] = {};
> -	char sizebuf[128] = {};
> -	const char *name = NULL;
> -	int sysfd;
> -	unsigned long long size = 0;
> -
> -	name = realpath(dev, path);
> -	if (!name)
> -		return -errno;
> -	name = path_basename(path);
> -
> -	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
> -	if (ret < 0)
> -		return ret;
> -	sysfd = open(sysfs, O_RDONLY);
> -	if (sysfd < 0)
> -		return -errno;
> -	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
> -	if (ret < 0) {
> -		close(sysfd);
> -		return ret;
> -	}
> -	errno = 0;
> -	size = strtoull(sizebuf, NULL, 10);
> -	if (size == ULLONG_MAX && errno == ERANGE) {
> -		close(sysfd);
> -		return -errno;
> -	}
> -	close(sysfd);
> -	*result_ret = size;
> -	return 0;
> -}
> -
>  int device_get_partition_size(const char *dev, u64 *result_ret)
>  {
>  	int fd = open(dev, O_RDONLY);
>  
>  	if (fd < 0)
> -		return device_get_partition_size_sysfs(dev, result_ret);
> +		return -errno;
>  
>  	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
>  		int ret = -errno;
> -- 
> 2.50.1
> 

Re: [PATCH 2/5] btrfs-progs: add error handling for device_get_partition_size()

[Qu Wenruo]

在 2025/8/6 04:38, Boris Burkov 写道:
> On Sat, Aug 02, 2025 at 04:25:48PM +0930, Qu Wenruo wrote:
>> The function device_get_partition_size() has all kinds of error paths,
>> but it has no way to return error other than returning 0.
>>
>> This is not helpful for end users to know what's going wrong.
>>
>> Change that function to have a dedicated return value for error
>> handling, and pass a u64 pointer for the result.
> 
> All the callers check ret < 0, rather than ret != 0, so any reason not
> to simply return the positive value in case of no error? Signed
> overflow?
> 
> In the kernel, BLK_DEV_MAX_SECTORS is set to LLONG_MAX >> 9, which I
> think means we are probably OK?

This sounds reasonable, I was under the impression that we need to 
preserve the full U64_MAX for block devices due to our on-disk format.

But if kernel has extra limits, I'm more than happier to use s64.

Thanks,
Qu

> 
>>
>> For most callers they are able to exit gracefully with a proper error
>> message.
>>
>> But for load_device_info(), we allow a failed size probing to continue
>> without 0 size, just as the old code.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   cmds/filesystem-usage.c |  9 +++++++--
>>   cmds/replace.c          | 14 ++++++++++++--
>>   common/device-utils.c   | 29 +++++++++++++++--------------
>>   common/device-utils.h   |  2 +-
>>   4 files changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
>> index f812af13482e..e367bffc3a01 100644
>> --- a/cmds/filesystem-usage.c
>> +++ b/cmds/filesystem-usage.c
>> @@ -820,8 +820,13 @@ static int load_device_info(int fd, struct array *devinfos)
>>   			strcpy(info->path, "missing");
>>   		} else {
>>   			strcpy(info->path, (char *)dev_info.path);
>> -			info->device_size =
>> -				device_get_partition_size((const char *)dev_info.path);
>> +			ret = device_get_partition_size((const char *)dev_info.path,
>> +							&info->device_size);
>> +			if (ret < 0) {
>> +				errno = -ret;
>> +				warning("failed to get device size for devid %llu: %m", dev_info.devid);
>> +				info->device_size = 0;
>> +			}
>>   		}
>>   		info->size = dev_info.total_bytes;
>>   		ndevs++;
>> diff --git a/cmds/replace.c b/cmds/replace.c
>> index 887c3251a725..d5b0b0e02ce1 100644
>> --- a/cmds/replace.c
>> +++ b/cmds/replace.c
>> @@ -269,7 +269,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>>   		strncpy_null((char *)start_args.start.srcdev_name, srcdev,
>>   			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
>>   		start_args.start.srcdevid = 0;
>> -		srcdev_size = device_get_partition_size(srcdev);
>> +		ret = device_get_partition_size(srcdev, &srcdev_size);
>> +		if (ret < 0) {
>> +			errno = -ret;
>> +			error("failed to get device size for %s: %m", srcdev);
>> +			goto leave_with_error;
>> +		}
>>   	} else {
>>   		error("source device must be a block device or a devid");
>>   		goto leave_with_error;
>> @@ -279,7 +284,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>>   	if (ret)
>>   		goto leave_with_error;
>>   
>> -	dstdev_size = device_get_partition_size(dstdev);
>> +	ret = device_get_partition_size(dstdev, &dstdev_size);
>> +	if (ret < 0) {
>> +		errno = -ret;
>> +		error("failed to get device size for %s: %m", dstdev);
>> +		goto leave_with_error;
>> +	}
>>   	if (srcdev_size > dstdev_size) {
>>   		error("target device smaller than source device (required %llu bytes)",
>>   			srcdev_size);
>> diff --git a/common/device-utils.c b/common/device-utils.c
>> index 783d79555446..6d89d16b029d 100644
>> --- a/common/device-utils.c
>> +++ b/common/device-utils.c
>> @@ -342,7 +342,7 @@ u64 device_get_partition_size_fd(int fd)
>>   	return result;
>>   }
>>   
>> -static u64 device_get_partition_size_sysfs(const char *dev)
>> +static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
>>   {
>>   	int ret;
>>   	char path[PATH_MAX] = {};
>> @@ -354,45 +354,46 @@ static u64 device_get_partition_size_sysfs(const char *dev)
>>   
>>   	name = realpath(dev, path);
>>   	if (!name)
>> -		return 0;
>> +		return -errno;
>>   	name = path_basename(path);
>>   
>>   	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
>>   	if (ret < 0)
>> -		return 0;
>> +		return ret;
>>   	sysfd = open(sysfs, O_RDONLY);
>>   	if (sysfd < 0)
>> -		return 0;
>> +		return -errno;
>>   	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>>   	if (ret < 0) {
>>   		close(sysfd);
>> -		return 0;
>> +		return ret;
>>   	}
>>   	errno = 0;
>>   	size = strtoull(sizebuf, NULL, 10);
>>   	if (size == ULLONG_MAX && errno == ERANGE) {
>>   		close(sysfd);
>> -		return 0;
>> +		return -errno;
>>   	}
>>   	close(sysfd);
>> -	return size;
>> +	*result_ret = size;
>> +	return 0;
>>   }
>>   
>> -u64 device_get_partition_size(const char *dev)
>> +int device_get_partition_size(const char *dev, u64 *result_ret)
>>   {
>> -	u64 result;
>>   	int fd = open(dev, O_RDONLY);
>>   
>>   	if (fd < 0)
>> -		return device_get_partition_size_sysfs(dev);
>> +		return device_get_partition_size_sysfs(dev, result_ret);
>> +
>> +	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
>> +		int ret = -errno;
>>   
>> -	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
>>   		close(fd);
>> -		return 0;
>> +		return ret;
>>   	}
>>   	close(fd);
>> -
>> -	return result;
>> +	return 0;
>>   }
>>   
>>   /*
>> diff --git a/common/device-utils.h b/common/device-utils.h
>> index cef9405f3a9a..bf04eb0f2fdd 100644
>> --- a/common/device-utils.h
>> +++ b/common/device-utils.h
>> @@ -42,7 +42,7 @@ enum {
>>    */
>>   int device_discard_blocks(int fd, u64 start, u64 len);
>>   int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
>> -u64 device_get_partition_size(const char *dev);
>> +int device_get_partition_size(const char *dev, u64 *result_ret);
>>   u64 device_get_partition_size_fd(int fd);
>>   u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
>>   int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
>> -- 
>> 2.50.1
>>

Re: [PATCH 5/5] btrfs-progs: remove device_get_partition_size_sysfs()

[Qu Wenruo]

在 2025/8/6 04:45, Boris Burkov 写道:
> On Sat, Aug 02, 2025 at 04:25:51PM +0930, Qu Wenruo wrote:
>> The helper is introduced for cases where the unprivileged user failed to
>> open the target file.
>>
>> The problem is, when such failure happens, it means the distro's file
>> mode or ACL is actively preventing unrelated users to access the raw
>> device.
>>
>> E.g. on my distro the default block device mode looks like this:
>>
>>    brw-rw---- 1 root disk 254, 32 Aug  2 13:35 /dev/vdc
>>
>> This means if an unprivileged end user is not in the disk group, it
>> should access the raw disk.
>>
>> Using sysfs as a fallback is more like a workaround, and the workaround
>> is soon getting out of control.
>>
>> For example the size is not in byte but in block unit. This is already
>> causing problem for issue #979.
> 
> shifting by sector size doesn't seem like a crazy thing for us to simply
> fix.
> 
>>
>> Furthermore to grab the correct size in bytes, we have to do all kinds
>> of sysfs probing to handle partitions (to get the block size from parent
>> node) and dm devices (directly from the current node).
> 
> I don't quite understand this justification, is this another fix we
> would have to make or is it what we are already doing?
> 
>>
>> With the recent error handling enhancement, I do not think we should
>> even bother using the sysfs fallback, the error message should be enough
>> to inform the end user.
> 
> I think that given we have users reporting being confused by size being
> off by a factor of 512, it makes more sense to make them happy by just
> shifting rather than throwing the whole thing out.

Yep, I'll drop the last patch.

Thanks,
Qu

> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   common/device-utils.c | 39 +--------------------------------------
>>   1 file changed, 1 insertion(+), 38 deletions(-)
>>
>> diff --git a/common/device-utils.c b/common/device-utils.c
>> index b52fbf33a539..7a0a299ccf83 100644
>> --- a/common/device-utils.c
>> +++ b/common/device-utils.c
>> @@ -333,49 +333,12 @@ int device_get_partition_size_fd_stat(int fd, const struct stat *st, u64 *result
>>   	return 0;
>>   }
>>   
>> -static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
>> -{
>> -	int ret;
>> -	char path[PATH_MAX] = {};
>> -	char sysfs[PATH_MAX] = {};
>> -	char sizebuf[128] = {};
>> -	const char *name = NULL;
>> -	int sysfd;
>> -	unsigned long long size = 0;
>> -
>> -	name = realpath(dev, path);
>> -	if (!name)
>> -		return -errno;
>> -	name = path_basename(path);
>> -
>> -	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
>> -	if (ret < 0)
>> -		return ret;
>> -	sysfd = open(sysfs, O_RDONLY);
>> -	if (sysfd < 0)
>> -		return -errno;
>> -	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>> -	if (ret < 0) {
>> -		close(sysfd);
>> -		return ret;
>> -	}
>> -	errno = 0;
>> -	size = strtoull(sizebuf, NULL, 10);
>> -	if (size == ULLONG_MAX && errno == ERANGE) {
>> -		close(sysfd);
>> -		return -errno;
>> -	}
>> -	close(sysfd);
>> -	*result_ret = size;
>> -	return 0;
>> -}
>> -
>>   int device_get_partition_size(const char *dev, u64 *result_ret)
>>   {
>>   	int fd = open(dev, O_RDONLY);
>>   
>>   	if (fd < 0)
>> -		return device_get_partition_size_sysfs(dev, result_ret);
>> +		return -errno;
>>   
>>   	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
>>   		int ret = -errno;
>> -- 
>> 2.50.1
>>
> 

Re: [PATCH 2/5] btrfs-progs: add error handling for device_get_partition_size()

[Boris Burkov]

On Wed, Aug 06, 2025 at 08:11:51AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/8/6 04:38, Boris Burkov 写道:
> > On Sat, Aug 02, 2025 at 04:25:48PM +0930, Qu Wenruo wrote:
> > > The function device_get_partition_size() has all kinds of error paths,
> > > but it has no way to return error other than returning 0.
> > > 
> > > This is not helpful for end users to know what's going wrong.
> > > 
> > > Change that function to have a dedicated return value for error
> > > handling, and pass a u64 pointer for the result.
> > 
> > All the callers check ret < 0, rather than ret != 0, so any reason not
> > to simply return the positive value in case of no error? Signed
> > overflow?
> > 
> > In the kernel, BLK_DEV_MAX_SECTORS is set to LLONG_MAX >> 9, which I
> > think means we are probably OK?
> 
> This sounds reasonable, I was under the impression that we need to preserve
> the full U64_MAX for block devices due to our on-disk format.
> 
> But if kernel has extra limits, I'm more than happier to use s64.

I looked into it a bit more. BLKGETSIZE64 is implemented as:

put_u64(argp, bdev_nr_bytes(bdev));

and bdev_nr_bytes() returns loff_t which is a signed long long. So I do
think it should be safe. Not to mention that needing the MSB of a 64 bit
integer to express a disk size is not happening in anyone's lifetime.

> 
> Thanks,
> Qu
> 
> > 
> > > 
> > > For most callers they are able to exit gracefully with a proper error
> > > message.
> > > 
> > > But for load_device_info(), we allow a failed size probing to continue
> > > without 0 size, just as the old code.
> > > 
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > ---
> > >   cmds/filesystem-usage.c |  9 +++++++--
> > >   cmds/replace.c          | 14 ++++++++++++--
> > >   common/device-utils.c   | 29 +++++++++++++++--------------
> > >   common/device-utils.h   |  2 +-
> > >   4 files changed, 35 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> > > index f812af13482e..e367bffc3a01 100644
> > > --- a/cmds/filesystem-usage.c
> > > +++ b/cmds/filesystem-usage.c
> > > @@ -820,8 +820,13 @@ static int load_device_info(int fd, struct array *devinfos)
> > >   			strcpy(info->path, "missing");
> > >   		} else {
> > >   			strcpy(info->path, (char *)dev_info.path);
> > > -			info->device_size =
> > > -				device_get_partition_size((const char *)dev_info.path);
> > > +			ret = device_get_partition_size((const char *)dev_info.path,
> > > +							&info->device_size);
> > > +			if (ret < 0) {
> > > +				errno = -ret;
> > > +				warning("failed to get device size for devid %llu: %m", dev_info.devid);
> > > +				info->device_size = 0;
> > > +			}
> > >   		}
> > >   		info->size = dev_info.total_bytes;
> > >   		ndevs++;
> > > diff --git a/cmds/replace.c b/cmds/replace.c
> > > index 887c3251a725..d5b0b0e02ce1 100644
> > > --- a/cmds/replace.c
> > > +++ b/cmds/replace.c
> > > @@ -269,7 +269,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
> > >   		strncpy_null((char *)start_args.start.srcdev_name, srcdev,
> > >   			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
> > >   		start_args.start.srcdevid = 0;
> > > -		srcdev_size = device_get_partition_size(srcdev);
> > > +		ret = device_get_partition_size(srcdev, &srcdev_size);
> > > +		if (ret < 0) {
> > > +			errno = -ret;
> > > +			error("failed to get device size for %s: %m", srcdev);
> > > +			goto leave_with_error;
> > > +		}
> > >   	} else {
> > >   		error("source device must be a block device or a devid");
> > >   		goto leave_with_error;
> > > @@ -279,7 +284,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
> > >   	if (ret)
> > >   		goto leave_with_error;
> > > -	dstdev_size = device_get_partition_size(dstdev);
> > > +	ret = device_get_partition_size(dstdev, &dstdev_size);
> > > +	if (ret < 0) {
> > > +		errno = -ret;
> > > +		error("failed to get device size for %s: %m", dstdev);
> > > +		goto leave_with_error;
> > > +	}
> > >   	if (srcdev_size > dstdev_size) {
> > >   		error("target device smaller than source device (required %llu bytes)",
> > >   			srcdev_size);
> > > diff --git a/common/device-utils.c b/common/device-utils.c
> > > index 783d79555446..6d89d16b029d 100644
> > > --- a/common/device-utils.c
> > > +++ b/common/device-utils.c
> > > @@ -342,7 +342,7 @@ u64 device_get_partition_size_fd(int fd)
> > >   	return result;
> > >   }
> > > -static u64 device_get_partition_size_sysfs(const char *dev)
> > > +static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
> > >   {
> > >   	int ret;
> > >   	char path[PATH_MAX] = {};
> > > @@ -354,45 +354,46 @@ static u64 device_get_partition_size_sysfs(const char *dev)
> > >   	name = realpath(dev, path);
> > >   	if (!name)
> > > -		return 0;
> > > +		return -errno;
> > >   	name = path_basename(path);
> > >   	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
> > >   	if (ret < 0)
> > > -		return 0;
> > > +		return ret;
> > >   	sysfd = open(sysfs, O_RDONLY);
> > >   	if (sysfd < 0)
> > > -		return 0;
> > > +		return -errno;
> > >   	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
> > >   	if (ret < 0) {
> > >   		close(sysfd);
> > > -		return 0;
> > > +		return ret;
> > >   	}
> > >   	errno = 0;
> > >   	size = strtoull(sizebuf, NULL, 10);
> > >   	if (size == ULLONG_MAX && errno == ERANGE) {
> > >   		close(sysfd);
> > > -		return 0;
> > > +		return -errno;
> > >   	}
> > >   	close(sysfd);
> > > -	return size;
> > > +	*result_ret = size;
> > > +	return 0;
> > >   }
> > > -u64 device_get_partition_size(const char *dev)
> > > +int device_get_partition_size(const char *dev, u64 *result_ret)
> > >   {
> > > -	u64 result;
> > >   	int fd = open(dev, O_RDONLY);
> > >   	if (fd < 0)
> > > -		return device_get_partition_size_sysfs(dev);
> > > +		return device_get_partition_size_sysfs(dev, result_ret);
> > > +
> > > +	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
> > > +		int ret = -errno;
> > > -	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
> > >   		close(fd);
> > > -		return 0;
> > > +		return ret;
> > >   	}
> > >   	close(fd);
> > > -
> > > -	return result;
> > > +	return 0;
> > >   }
> > >   /*
> > > diff --git a/common/device-utils.h b/common/device-utils.h
> > > index cef9405f3a9a..bf04eb0f2fdd 100644
> > > --- a/common/device-utils.h
> > > +++ b/common/device-utils.h
> > > @@ -42,7 +42,7 @@ enum {
> > >    */
> > >   int device_discard_blocks(int fd, u64 start, u64 len);
> > >   int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
> > > -u64 device_get_partition_size(const char *dev);
> > > +int device_get_partition_size(const char *dev, u64 *result_ret);
> > >   u64 device_get_partition_size_fd(int fd);
> > >   u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
> > >   int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
> > > -- 
> > > 2.50.1
> > > 
>