fuse.ko: Add extattrs support.

[fsu]

Summary

This diff is modified version of @ken's patch from here:
https://docs.freebsd.org/cgi/mid.cgi?CD5FCB90-1952-4014-BBE0-1BFF1EF85E17

The difference from original is, that additional sysctl added to turn on/off extattrs namespaces.

Test Plan

Was tested with e2fsprogs and tuxera ntfs-3g fuse drivers.

Please let me know (@fsu) if you want to try it locally, because it is not so obvious how to build and install fuse fs drivers and activate extattrs functionality.

[cem]

Please upload a diff with full context.

It can be done with either the 'arc diff' tool, or creating a diff with the '-U999999' option.

Thanks!

[pfg]

Related good news: thanks to mandree@ , the sysutils/e2fsprogs now has a FUSEFS option to build the fuse support.

[fsu]

Update diff to have full context.

[cem]

Thanks for doing this work!

Here are some mostly stylistic suggestions. Many of these comments in one place apply to all or most of the added routines.

In fact, there seems to be quite a bit of shared code. Maybe some of it could be deduplicated?

I didn't examine the in-place attribute list conversion at all.

sys/fs/fuse/fuse_vnops.c

195

I know it isn't the pattern in this file, but it might be good to have a descriptive string about what this sysctl does.

Also — why not always prefix?

2013

It definitely seems like a weakness that this is a system-wide option, instead of per mount or whatever. Is it not possible to pass the attrnamespace up to FUSE drivers out of band?

2028

Why not just fdisp_make_vp(&fdi, ..., vp, ...)?

2042

It seems like there is a time-of-check, time-of-use (TOCTOU) race here. Probably we should check and cache the sysctl value once at the top of the function.

2043

This line is common to both cases and can be moved outside the if.

2045

The output of snprintf is already nul-terminated.

2048

This one could just be memcpy()

2077

Don't need to cast answ (void *).

2156

Probably should not be a raw printf

2178

I don't see much benefit to conversion in place. Seems like it would be a bit more straightforward to convert into a separate buffer, with no obvious drawbacks.

2292

This is unused before it is assigned again below fdisp_wait_answ

[fsu]

Here are some fixes according @cem's suggestions.

[fsu]

The diff updated.
Also, it would be great to decide something with this new namespaces sysctl, leave it or remove.
May be somebody know some experienced persons, who worked with fuse fs, and can add them to this review, to clarify this question.

sys/fs/fuse/fuse_vnops.c

195

I decided, that the fuse fs is too generic to pass namespace prefixes every time, because fuse have sometimes really unexpected applications. Something like mp3fs for example. So leave the ability for user to turn it off.
From other side, as you marked in your comment below, that it is system wide, but should be like mount point option.
And I can not provide the usage example, where namespace prefixes turning off is required for now. It mean that we can completely remove it.
May be ask somebody other... so we need third opinion.

2028

Fixed.

2077

Fixed.

2156

Did not clearly understood what mean raw printf, so the func macro was removed.

2178

I completely agree that logic is not obvious, but as you can see, there no any malloc calls in this file, so just leave this tradition may be.

2292

Fixed.

[pfg]

Thanks cem@ for the review.

The problem I see with the sysctl is that we have too many of them; they are generally undocumented, and it's ultimately unlikely people will spend time on it. A default value ("fusefs" perhaps?) would be good to have.

Another thing to consider, but that doesn't have any relationship with committing this patch, is if/how fuse can work in the linuxulator and if we can make the linuxulator handle extended attributes.

[cem]

Changes from v2 to v3 look mostly good!

In D12485#259853, @pfg wrote:

The problem I see with the sysctl is that we have too many of them; they are generally undocumented, and it's ultimately unlikely people will spend time on it. A default value ("fusefs" perhaps?) would be good to have.

I think the main problem is it configures globally a setting that is most useful per mount. But maybe I am overestimating the number of FUSE mounts people have.

Another thing to consider, but that doesn't have any relationship with committing this patch, is if/how fuse can work in the linuxulator and if we can make the linuxulator handle extended attributes.

I think that already works, or just requires work on the linuxulator end (after this patch). With this change, fusefs implements the generic vnode API for extattrs.

sys/fs/fuse/fuse_vnops.c

192–194

Should be bool / SYSCTL_BOOL.

2002

Should be bool (not err)

2156

Generally VOPs should not invoke printf() in error cases. It is ok to have a conditional debug_printf(), though. __func__ is totally fine.

2178

There's a first time for everything ;-).

2390

style(9) was correct before this change -- integer error values should be compared against zero rather than treated as a boolean value

[pfg]

In D12485#259905, @cem wrote:

Changes from v2 to v3 look mostly good!

In D12485#259853, @pfg wrote:

The problem I see with the sysctl is that we have too many of them; they are generally undocumented, and it's ultimately unlikely people will spend time on it. A default value ("fusefs" perhaps?) would be good to have.

I think the main problem is it configures globally a setting that is most useful per mount. But maybe I am overestimating the number of FUSE mounts people have.

I don't think currently the main consumer for ntfs driver and EAs currently don't translate very well between those. Ken's work was probably related to ceph, which like does use such attributes.

Another thing to consider, but that doesn't have any relationship with committing this patch, is if/how fuse can work in the linuxulator and if we can make the linuxulator handle extended attributes.

I think that already works, or just requires work on the linuxulator end (after this patch). With this change, fusefs implements the generic vnode API for extattrs.

It might be that we are implementing a very old version of the fuse API/protocol. looked a bit at updating it but it wants to control locking and it may not translate at all to FreeBSD. Just a consideration for the future.

I will be glad to approve the next iteration of this patch.

[cem]

In D12485#259922, @pfg wrote:

I will be glad to approve the next iteration of this patch.

FWIW, I haven't reviewed fuse_xattrlist_convert at all because it looks "complicated" and still have hesitations about doing it in-place.

[ken]

In D12485#259922, @pfg wrote:

In D12485#259905, @cem wrote:

Changes from v2 to v3 look mostly good!

In D12485#259853, @pfg wrote:

The problem I see with the sysctl is that we have too many of them; they are generally undocumented, and it's ultimately unlikely people will spend time on it. A default value ("fusefs" perhaps?) would be good to have.

I think the main problem is it configures globally a setting that is most useful per mount. But maybe I am overestimating the number of FUSE mounts people have.

I don't think currently the main consumer for ntfs driver and EAs currently don't translate very well between those. Ken's work was probably related to ceph, which like does use such attributes.

I originally wrote it for LTFS, to allow getting/setting extended attributes there.

[ken]

sys/fs/fuse/fuse_vnops.c

195

At least from a FreeBSD standpoint, I think that the attributes that come back in the list are always sent without a prefix. So, wouldn't providing them with a prefix in the list break existing FreeBSD software?

2178

The original version worked fine. This version does the fix in a single pass instead of two passes, which is more efficient.

Whether you do it in place or with a separate buffer you're going to have similar logic. So we need to review the logic in detail and make sure it is correct.

But, my question is, why make the prefix removal optional? Are there FreeBSD applications that are looking for the list to come back with prefixes? Will they break if the list comes back with prefixes?

[fsu]

• remove extattr namespaces sysctl.
• revet err check according style(9)
• swap printf() to debug_printf()

[fsu]

sys/fs/fuse/fuse_vnops.c

2178

Please, fill free to provide any alternative implementation. I ready to review and test it.

[pfg]

In D12485#259923, @cem wrote:

In D12485#259922, @pfg wrote:

I will be glad to approve the next iteration of this patch.

FWIW, I haven't reviewed fuse_xattrlist_convert at all because it looks "complicated" and still have hesitations about doing it in-place

I don't see another alternative. FWIW, I asked Fedor to look at ken's patch because he has done essentially the same for ext2/3/4.
If you find a better way the ext2fs code can make use of it too :).

[pfg]

In D12485#260056, @thisisadrgreenthumb_gmail.com wrote:

• remove extattr namespaces sysctl.

I think this is a sensible approach. We can always put it back in if there is a reasonable usecase.

I think it looks good but Fedor will be on vacation so we can still think it over for a week or two.

[cem]

In D12485#260120, @pfg wrote:

I don't see another alternative. FWIW, I asked Fedor to look at ken's patch because he has done essentially the same for ext2/3/4.
If you find a better way the ext2fs code can make use of it too :).

My suggestion is to use separate input and output buffers. Just removes some things that can go wrong rewriting in place.

In D12485#260121, @pfg wrote:

I think it looks good but Fedor will be on vacation so we can still think it over for a week or two.

Ok.

[cem]

sys/fs/fuse/fuse_vnops.c

2161

I'd like to see the arguments documented in the comment as well.

2171

style nit: bcmp(...) == 0

2173–2174

Need to validate len <= 255 here? And aren't we scribbling on the extattr name at this point for unprefixed strings?

2270

This cast of answ is not needed.

[fsu]

• Swap inplace names conversion to malloced.
• Reorder names conversion function arguments.
• Fix style.

[fsu]

sys/fs/fuse/fuse_vnops.c

2161

Could you please provide me some arguments comment example, which I can borrow to use with this function.

2173–2174

We not need to validate name length anyhow.
It will be done by fuse file-system implementation.

[cem]

sys/fs/fuse/fuse_vnops.c

2161

Not sure what you mean. I just want you to describe what each input parameter is.

2173–2174

We need to validate lengths in the kernel. We can't just trust userspace filesystems.

[fsu]

• Add name length check.
• Add comments to function arguments.
• Some refactoring.

[pfg]

I will be AFK starting tomorrow ... looks good to me.

[cem]

We lost the context again, but that's ok. Looks functionally good to me. Some stylistic nits below, then it is ready to commit. :-)

sys/fs/fuse/fuse_vnops.c

195

Probably "." should be "%c", with extattr_namespace_separator argument?

2156

Usual style is to put the argument comments in the block comment above the function.

2167

style nit: pos_next isn't a position or index; it's a length or distance. Maybe call it dist_to_next or len_name or something like that.

2173

Maybe do the inverse of this check before assigning len to bsd_list[...].

Moving the check before assignment means we maybe avoid spurious static analyzer warnings about bogus length values being truncated to char.

And inverting the sense of the check allows us to drop a level of indentation for the happy path.

2330

Cast is not needed.

[fsu]

Everything noticed above will be fixed before commit.
Conrad, I should say you thanks for your review and time, which you spent.
Also, it would be great if you can, not review, but just look to ext2fs extents diff, where you was invited.