Page MenuHomeFreeBSD

fuse.ko: Add extattrs support.
ClosedPublic

Authored by fsu on Sep 24 2017, 3:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 8:57 AM
Unknown Object (File)
Jan 12 2024, 10:07 AM
Unknown Object (File)
Dec 20 2023, 7:17 AM
Unknown Object (File)
Dec 10 2023, 10:16 PM
Unknown Object (File)
Nov 8 2023, 10:35 AM
Unknown Object (File)
Nov 7 2023, 12:25 AM
Unknown Object (File)
Nov 6 2023, 11:08 PM
Unknown Object (File)
Nov 5 2023, 11:16 AM
Subscribers

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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!

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

Update diff to have full context.

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 ↗(On Diff #33394)

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 ↗(On Diff #33394)

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 ↗(On Diff #33394)

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

2042 ↗(On Diff #33394)

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 ↗(On Diff #33394)

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

2045 ↗(On Diff #33394)

The output of snprintf is already nul-terminated.

2048 ↗(On Diff #33394)

This one could just be memcpy()

2077 ↗(On Diff #33394)

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

2156 ↗(On Diff #33394)

Probably should not be a raw printf

2178 ↗(On Diff #33394)

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 ↗(On Diff #33394)

This is unused before it is assigned again below fdisp_wait_answ

Here are some fixes according @cem's suggestions.

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 ↗(On Diff #33394)

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 ↗(On Diff #33394)

Fixed.

2077 ↗(On Diff #33394)

Fixed.

2156 ↗(On Diff #33394)

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

2178 ↗(On Diff #33394)

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 ↗(On Diff #33394)

Fixed.

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.

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 ↗(On Diff #33528)

Should be bool / SYSCTL_BOOL.

2002 ↗(On Diff #33528)

Should be bool (not err)

2390 ↗(On Diff #33528)

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

2156 ↗(On Diff #33394)

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

2178 ↗(On Diff #33394)

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

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.

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.

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.

sys/fs/fuse/fuse_vnops.c
195 ↗(On Diff #33394)

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 ↗(On Diff #33394)

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?

  • remove extattr namespaces sysctl.
  • revet err check according style(9)
  • swap printf() to debug_printf()
sys/fs/fuse/fuse_vnops.c
2178 ↗(On Diff #33394)

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

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 :).

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.

This revision is now accepted and ready to land.Sep 29 2017, 2:02 PM
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.

sys/fs/fuse/fuse_vnops.c
2161 ↗(On Diff #33552)

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

2171 ↗(On Diff #33552)

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

2173–2174 ↗(On Diff #33552)

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

2270 ↗(On Diff #33552)

This cast of answ is not needed.

  • Swap inplace names conversion to malloced.
  • Reorder names conversion function arguments.
  • Fix style.
This revision now requires review to proceed.Oct 9 2017, 7:55 AM
sys/fs/fuse/fuse_vnops.c
2161 ↗(On Diff #33552)

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

2173–2174 ↗(On Diff #33552)

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

sys/fs/fuse/fuse_vnops.c
2161 ↗(On Diff #33552)

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

2173–2174 ↗(On Diff #33552)

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

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

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

This revision is now accepted and ready to land.Oct 11 2017, 3:33 PM

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 ↗(On Diff #33861)

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

2156 ↗(On Diff #33861)

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

2167 ↗(On Diff #33861)

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 ↗(On Diff #33861)

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 ↗(On Diff #33861)

Cast is not needed.

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.

This revision was automatically updated to reflect the committed changes.