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

[胡玮文 <sehuww@mail.scut.edu.cn>]

Hi Xiang,

> 在 2021年1月22日，08:34，Gao Xiang <hsiangkao@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@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? 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?

> 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
>>