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

[Huang Jianan]

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 <w...@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);