On 8/12/25 12:32 PM, Rajeev Mishra wrote:
> Hi Kuai,
>
> Thank you for the feedback on the v2 patch regarding error handling.
>
> Yu mentioned:
>> return 0 here is odd. Why not "return ret;" to propagate the error if any ?
>
> I understand the concern about proper error propagation. However, there's a
> type compatibility issue I'd like to discuss before implementing v3:
>
> 1. Current function signature: `static loff_t get_size(...)`
> - Returns size as positive loff_t (unsigned 64-bit)
> - All callers expect non-negative size values
>
> 2. vfs_getattr_nosec() error codes are negative integers (-ENOENT, -EIO, etc.)
> - Returning `ret` would cast negative errors to huge positive numbers
> - This could cause loop devices to appear as exabyte-sized
>
> 3. Current callers like loop_set_size() don't handle error checking
>
> Would you prefer for v3:
> a) Change function signature to `int get_size(..., loff_t *size)` and update all callers
> b) Different approach?
>
> diff with ret approach
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c418c47db76e..15117630c6c1 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -142,12 +142,13 @@ static int part_shift;
> * @offset: offset into the backing file
> * @sizelimit: user-specified size limit
> * @file: the backing file
> + * @size: pointer to store the calculated size
> *
> * Calculate the effective size of the loop device
> *
> - * Returns: size in 512-byte sectors, or 0 if invalid
> + * Returns: 0 on success, negative error code on failure
> */
> -static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
Since loff_t is "long long", so a signed type, I would keep this interface and
add a negative error check in the 2 call sites for get_size(). That is simpler.
> +static int get_size(loff_t offset, loff_t sizelimit, struct file *file, loff_t *size)
> {
> struct kstat stat;
> loff_t loopsize;
> @@ -159,7 +160,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
> */
> ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
> if (ret)
> - return 0;
> + return ret;
>
> loopsize = stat.size;
>
> @@ -167,7 +168,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
> loopsize -= offset;
> /* offset is beyond i_size, weird but possible */
> if (loopsize < 0)
> - return 0;
> + return -EINVAL;
>
> if (sizelimit > 0 && sizelimit < loopsize)
> loopsize = sizelimit;
> @@ -175,12 +176,20 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
> * Unfortunately, if we want to do I/O on the device,
> * the number of 512-byte sectors has to fit into a sector_t.
> */
> - return loopsize >> 9;
> + *size = loopsize >> 9;
> + return 0;
> }
>
> static loff_t get_loop_size(struct loop_device *lo, struct file *file)
> {
> - return get_size(lo->lo_offset, lo->lo_sizelimit, file);
> + loff_t size;
> + int ret;
> +
> + ret = get_size(lo->lo_offset, lo->lo_sizelimit, file, &size);
> + if (ret)
> + return 0; /* Fallback to 0 on error for backward compatibility */
> +
> + return size;
> }
>
>
> I am happy to implement whichever direction you think is best.
>
> Thanks,
> Rajeev
--
Damien Le Moal
Western Digital Research
