[PATCH] erofs-utils: fuse: fix random readlink error

[Gao Xiang]

Hi Weiwen,

On Fri, Jan 22, 2021 at 09:00:44AM +0800, 胡玮文 wrote:
> Hi Xiang,
> 
> > 在 2021年1月22日，08:34，Gao Xiang <hsiangkao at redhat.com> 写道：
> > 
> > Hi Weiwen,
> > 
> >> On Fri, Jan 22, 2021 at 12:31:43AM +0800, Hu Weiwen wrote:
> >> readlink should fill a **null terminated** string in buffer.
> >> 
> >> Also, read should return number of bytes remaining on EOF.
> >> 
> >> Link: https://lore.kernel.org/linux-erofs/20210121101233.GC6680@DESKTOP-N4CECTO.huww98.cn/
> >> Signed-off-by: Hu Weiwen <sehuww at mail.scut.edu.cn>
> > 
> > Thanks for catching this!
> > 
> >> ---
> >> fuse/main.c | 14 +++++++++++++-
> >> 1 file changed, 13 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/fuse/main.c b/fuse/main.c
> >> index c162912..bc1e496 100644
> >> --- a/fuse/main.c
> >> +++ b/fuse/main.c
> >> @@ -71,6 +71,12 @@ static int erofsfuse_read(const char *path, char *buffer,
> >>    if (ret)
> >>        return ret;
> >> 
> >> +    if (offset >= vi.i_size)
> >> +        return 0;
> >> +
> >> +    if (offset + size > vi.i_size)
> >> +        size = vi.i_size - offset;
> >> +
> > 
> > It would be better to call erofs_pread() with the original offset
> > and size (also I think there is some missing memset(0) for
> > !EROFS_MAP_MAPPED), and fix up the return value to the needed value.
> 
> Yes, that is what I have initially tried. But this way is harder than I thought. 
> EROFS_MAP_MAPPED flag seems to be designed to handle sparse file, and is reused to
> represent EOF. So maybe we need a new flag to represent EOF? 

Nope, I think we just need to handle return value of read, I mean

erofs_ilookup()

ret = erofs_pread()
if (ret)
	return ret;

if (offset + size > vi.i_size)
	return vi.i_size - offset;

return size;

Is that enough? Am I missing something?

> So that we can distinguish EOF and hole in the middle, and return the bytes read.
> Or we just abandon the sparse file support, and use EROFS_MAP_MAPPED to indicate
> EOF exclusively. Since erofs current does not actually support this, right?

Actually, Pratik was working on it months ago, if you have some interest
of uncompressed sparse files (since for compressed files, 0-ed data would
be highly compressed by fixed-sized output compression.), could you pick
his feature up if possible? That would be of great help to EROFS as long
as you have some interest and extra time... :)

https://lore.kernel.org/r/20200102094732.31567-1-pratikshinde320@gmail.com/

Thanks,
Gao Xiang

> 
> > Thanks,
> > Gao Xiang
> > 
> >>    ret = erofs_pread(&vi, buffer, size, offset);
> >>    if (ret)
> >>        return ret;
> >> @@ -79,10 +85,16 @@ static int erofsfuse_read(const char *path, char *buffer,
> >> 
> >> static int erofsfuse_readlink(const char *path, char *buffer, size_t size)
> >> {
> >> -    int ret = erofsfuse_read(path, buffer, size, 0, NULL);
> >> +    int ret;
> >> +    size_t path_len;
> >> +
> >> +    erofs_dbg("path:%s size=%zd", path, size);
> >> +    ret = erofsfuse_read(path, buffer, size, 0, NULL);
> >> 
> >>    if (ret < 0)
> >>        return ret;
> >> +    path_len = min(size - 1, (size_t)ret);
> >> +    buffer[path_len] = '\0';
> >>    return 0;
> >> }
> >> 
> >> -- 
> >> 2.30.0
> >> 
>