[PATCH v9] erofs-utils: add support for fuse 2/3 lowlevel API

[Li Yiyan]

Hi Xiang:

Thanks for your thoughtful comments, here are my replies. 

On 2023/9/17 00:13, Gao Xiang wrote:
> 
> 
> On 2023/9/3 19:38, Li Yiyan wrote:
>> Hi Xiang:

...

>> +struct erofsfuse_readdir_context {
>>       struct erofs_dir_context ctx;
>> -    fuse_fill_dir_t filler;
>> -    struct fuse_file_info *fi;
>> +
>> +    fuse_req_t req;
>>       void *buf;
>> +    int is_plus;
>> +    size_t offset;
>> +    size_t buf_size;
> 
> please rename it as buf_rem;
> 

Thanks.

>> +    size_t start_off;
>> +    struct fuse_file_info *fi;
>> +};
>> +
>> +
>> +static int erofsfuse_add_dentry(struct erofs_dir_context *ctx)
>>   {
>> -    struct erofsfuse_dir_context *fusectx = (void *)ctx;
>> -    struct stat st = {0};
>> +    size_t size = 0;
> 
> please rename it as entsize,  or see:
> https://github.com/libfuse/libfuse/blob/master/example/passthrough_ll.c#L665
> 
> There are many `size`s here, I'm not sure which size you're meaning.
> 

It's acutally entsize, I'll fix it in V10.

>>       char dname[EROFS_NAME_LEN + 1];
>> +    struct erofsfuse_readdir_context *readdir_ctx = (void *)ctx;
>> +
>> +    if (readdir_ctx->offset < readdir_ctx->start_off) {
>> +        readdir_ctx->offset +=
>> +            ctx->de_namelen + sizeof(struct erofs_dirent);
> 
> I still don't think it's right.
> 
> First, if you treat it as an real offset of directory file, this
>        calculation is incorrect.  There are many reasons, the
>        obvious one is that "each directory block may have trailing
>        padding data"
> 
> Second, if you just treat it as an "index", for example.  Then,
>         I don't really understand why you rename
> 
>       readdir_ctx->offset => readdir_ctx->index
>       readdir_ctx->start_off => readdir_ctx->offset
> 
> and make readdir_ctx->index add by one for each iteration?
> 
> Anyway, this part seems quite odd to me.

You are right, in fact readdir_ctx->offset is a marker to
identify the current point in the directory stream. Rename it
to index and add one for each iteration indeed avoids
confusing. I'll fix it in V10.

>> +        st.st_mode = erofs_ftype_to_mode(ctx->de_ftype, 0);
>> +        st.st_ino = ctx->de_nid;
> 
> I think it's buggy here, I think you need to:
>         st.st_ino = erofsfuse_to_ino(ctx->de_nid);
> 

Thanks.
...

>> +        size = fuse_add_direntry_plus(readdir_ctx->req,
>> +                          readdir_ctx->buf,
>> +                          readdir_ctx->buf_size, dname,
>> +                          &param, readdir_ctx->offset);
> 
> Please
> 
>   #else
>         return -EOPNOTSUPP;
> 
> here.
> 

Thanks.

>> +static inline void erofsfuse_readdir_general(fuse_req_t req, fuse_ino_t ino,
>> +                         size_t size, off_t off,
>> +                         struct fuse_file_info *fi,
>> +                         int plus)
>> +{
>> +    int ret = 0;
>> +    char *buf = NULL;
>> +    struct erofsfuse_readdir_context ctx;
> 
> I'd suggest to use
>     struct erofsfuse_readdir_context ctx = {};
> 
> to avoid potential uninitialization.

Thanks.
>> -    default:
>> -        DBG_BUGON(1);
>> -        break;
>> +        if (!strncmp(arg, "-h", 2))
>> +            fusecfg.show_help = true;
>> +        if (!strncmp(arg, "-V", 2))
>> +            fusecfg.show_version = true;
> 
> Why you need to change this from strcmp to strncmp()?

I'll change back to `strcmp`.

> 
> 
> Besides, please change all debugging messages into:
> 
> erofs_dbg("read (%llu): size = %zu, off = %lu", ino | 0ULL, size, off);
> ..
> erofs_dbg("getxattr(%llu): name = %s, size = %zu",
>       ino | 0ULL, name, size);
> ..
> erofs_dbg("listxattr(%llu): size = %zu", ino | 0ULL, size);

Thanks.

> 
> 
> Finally, I try this commit with linux-6.1.53 source code and _libfuse2_,
> and I got several false corruptions like below:
> 
> $fssum -MACUG mnt
> stat failed for mnt//Documentation/ABI: Structure needs cleaning
> 
> [xize.gx at e69b19392.et15sqa /home/xize.gx/erofs-utils]
> $fssum -MACUG mnt
> stat failed for mnt//Documentation/ABI: Structure needs cleaning
> 
> [xize.gx at e69b19392.et15sqa /home/xize.gx/erofs-utils]
> $fssum -MACUG mnt
> stat failed for mnt//Documentation/.gitignore: Structure needs cleaning
> 
> [xize.gx at e69b19392.et15sqa /home/xize.gx/erofs-utils]
> $fssum -MACUG mnt
> stat failed for mnt//LICENSES/deprecated: Structure needs cleaning
> 
> [xize.gx at e69b19392.et15sqa /home/xize.gx/erofs-utils]
> $fssum -MACUG mnt
> stat failed for mnt//drivers/staging/most: Structure needs cleaning
> 
> [xize.gx at e69b19392.et15sqa /home/xize.gx/erofs-utils]
> $fssum -MACUG mnt
> stat failed for mnt//drivers/staging/most: Structure needs cleaning
> 
> [xize.gx at e69b19392.et15sqa /home/xize.gx/erofs-utils]
> $fssum -MACUG mnt
> stat failed for mnt//tools/testing/selftests/drivers/net/bonding/settings: Structure needs cleaning
> 
> I don't think it's a correct sign.  I try to debug more but
> it seems more issues here, I gave up eveuatually.

Fixed in V10. The reason is uninitialized struct in erofsfuse_lookup.

Also solve a memory leak problem in V10, now no memory leak occurs
during a over 10 mins fssum process.

> 
> Thanks,
> Gao Xiang
> 

Thanks,
Yiyan