Re: [PATCH RFC 2/8] fs: always get the file size in _fs_read()

[Huang Jianan <jnhuang95@gmail.com>]

Hi, wenruo,

在 2022/6/28 15:28, Qu Wenruo 写道:
> For _fs_read(), @len == 0 means we read the whole file.
> And we just pass @len == 0 to make each filesystem to handle it.
>
> In fact we have info->size() call to properly get the filesize.
>
> So we can not only call info->size() to grab the file_size for len == 0
> case, but also detect invalid @len (e.g. @len > file_size) in advance or
> truncate @len.
>
> This behavior also allows us to handle unaligned better in the incoming
> patches.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/fs.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 6de1a3eb6d5d..d992cdd6d650 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -579,6 +579,7 @@ static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>   {
>   	struct fstype_info *info = fs_get_info(fs_type);
>   	void *buf;
> +	loff_t file_size;
>   	int ret;
>   
>   #ifdef CONFIG_LMB
> @@ -589,10 +590,26 @@ static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>   	}
>   #endif
>   
> -	/*
> -	 * We don't actually know how many bytes are being read, since len==0
> -	 * means read the whole file.
> -	 */
> +	ret = info->size(filename, &file_size);

I get an error when running the erofs test cases. The return value isn't 
as expected
when reading symlink file.
For symlink file, erofs_size will return the size of the symlink itself 
here.
> +	if (ret < 0) {
> +		log_err("** Unable to get file size for %s, %d **\n",
> +				filename, ret);
> +		return ret;
> +	}
> +	if (offset >= file_size) {
> +		log_err(
> +		"** Invalid offset, offset (%llu) >= file size (%llu)\n",
> +			offset, file_size);
> +		return -EINVAL;
> +
> +	}
> +	if (len == 0 || offset + len > file_size) {
> +		if (len > file_size)
> +			log_info(
> +	"** Truncate read length from %llu to %llu, as file size is %llu **\n",
> +				 len, file_size, file_size);
> +		len = file_size - offset;
Then, we will get a wrong len in the case of len==0. So I think we need 
to do something
extra with the symlink file?

Thanks,
Jianan
> +	}
>   	buf = map_sysmem(addr, len);
>   	ret = info->read(filename, buf, offset, len, actread);
>   	unmap_sysmem(buf);