fuse extattrs: fix issue when neither uio nor size were not passed to VOP_* (logic only).

[fsu]

Summary

Split diff for https://reviews.freebsd.org/D13487.

Test Plan

The fuse extattrs tests will be included to test plan for https://reviews.freebsd.org/D13209
when linuxulator xattrs will be finished.

[pfg]

Please add a description here to have an idea what the commit log would look like.

[pfg]

LGTM

[pfg]

fs/fuse/fuse_vnops.c

2218 ↗

(On Diff #36706)

Perhaps rename to fuse_list_len: they are, after all, not necessarily from linux.

[cem]

fs/fuse/fuse_vnops.c

2218 ↗

(On Diff #36706)

I think it's ok. FUSE ABI uses linux-style attributes, and that's all this refers to.

2270–2271 ↗

(On Diff #36706)

I'm not sure I understand what's going on here. Where is the linux_list_len buffer that the response is read into?

2285 ↗

(On Diff #36706)

should this fdi.iosize be linux_list_len?

[fsu]

Add linux_list stack variable to make listxattr() function more readable.

[cem]

fs/fuse/fuse_vnops.c

2270–2271 ↗

(On Diff #36756)

So get_xattr_in was initialized to fdi.indata for the previous instance of fdi. Is it valid to retain the pointer inside the fdi structure over a fdisk_make_vp() call?

(Cosmetic nit: should it be list_xattr_in rather than get_xattr_in?)

2274–2275 ↗

(On Diff #36756)

Is this even possible, given we've already successfully invoked FUSE_LISTXATTR earlier in this function?

[fsu]

More deep reinitialization of struct fuse_dispatcher fdi before second call to fdisp_wait_answ() under fuse_vnop_listextattr().

[fsu]

fs/fuse/fuse_vnops.c

2270–2271 ↗

(On Diff #36756)

So get_xattr_in was initialized to fdi.indata for the previous instance of fdi. Is it valid to retain the pointer inside the fdi structure over a fdisk_make_vp() call?

Yep, as I can see, here fdi.indata could be reallocated in the fdisp_make_vp() -> fdisp_make_pid() -> FUSE_DIMALLOC() in case of fdi.iosize.

(Cosmetic nit: should it be list_xattr_in rather than get_xattr_in?)

Not sure, because get_xattr_in has type struct fuse_getxattr_in, so it could be some sort of confusion, comparing with getxattr() path.
But, from other point of view it is possible to add struct fuse_listxattr_in to fuse_kernel.h additionally to struct fuse_getxattr_in and struct fuse_setxattr_in. So, if you prefer second option, I ready to make this update.

2274–2275 ↗

(On Diff #36756)

I prefer to leave this check, in case, for example, on force unmount (mean when usb drive disconnected without unmount command call).

[cem]

fs/fuse/fuse_vnops.c

2270–2271 ↗

(On Diff #36756)

Leaving it as get_xattr_in for now is ok with me.

If you want to do that cleanup, though, I'm not opposed.

2274–2275 ↗

(On Diff #36756)

Would the underlying layer return ENOSYS in that case, or another error (ENXIO / EIO)?

[fsu]

• Declare struct fuse_listxattr_* to not use fuse_getxattr_* in case of list().
• Remove ENOSYS error check in case of second fdisp_wait_answ() call under list().

[fsu]

fs/fuse/fuse_vnops.c

2274–2275 ↗

(On Diff #36756)

Aha... the ENOSYS is redundant here.
But, in case of force unmount or another troubles with drive, it is difficult to say which error will be returned. Because, as I understand, it should be returned by underlying fuse filesystem after block read/write calls.

[cem]

LGTM. I am not sure whether removing answ/iosize checks (both locations) is safe.

sys/fs/fuse/fuse_vnops.c

2287–2288 ↗

(On Diff #37667)

Is it safe to remove these checks?

[pfg]

(I thought this had been committed already :-/ )

[fsu]

sys/fs/fuse/fuse_vnops.c

2287–2288 ↗

(On Diff #37667)

Ok, I spent some time with the source code inspecting and the question was, is it possible to have the fdi.answ == NULL in case the fdisp_wait_answ()
returned 0:

• fdisp_wait_answ():

fdip->answ = fticket_resp(fdip->tick)->base;

• fticket_resp():

return (&ftick->tk_aw_fiov);

The tk_aw_fiov come from:
struct fuse_ticket {
...
/* fields for handling answers coming from userspace */

struct fuse_iov              tk_aw_fiov;

...
};

The tk_aw_fiov is initialized inside the fticket_init(void *mem, int size, int flags) function
using:
fiov_init(&ftick->tk_aw_fiov, 0);
As could be seen in the code of this function, the malloc called to base pointer and the size come from
uint32_t msize = FU_AT_LEAST(size);
and it will be not equal 0.
So, as I understand, if the struct fuse_ticket in the struct fuse_dispatcher was initialized, it is guaranteed, that fdi.answ != NULL after successfull fdisp_wait_answ() call.

Also, the same check in the fuse_vnop_getextattr() will be removed before landing.

[john_john-millikin.com]

Sorry for the drive-by review, I found this review when writing an article about FUSE.

head/sys/fs/fuse/fuse_kernel.h

287

Shouldn't this line be __u32 padding, to match the previous use of fuse_getxattr_in for FUSE_LISTXATTR? Otherwise it's deviating from the ABI of mainline FUSE.

292

I think this should also be padding, to match fuse_getxattr_out.

[cem]

head/sys/fs/fuse/fuse_kernel.h

287

Perhaps the name should match for stylistic reasons (or perhaps not, I don't know), but the name of a C struct member variable does not affect wire protocol ABI in any way.