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.
Details
Details
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
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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!
Comment Actions
Related good news: thanks to mandree@ , the sysutils/e2fsprogs now has a FUSEFS option to build the fuse support.
Comment Actions
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 | |
Comment Actions
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. | |
| 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. | |
Comment Actions
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.
Comment Actions
Changes from v2 to v3 look mostly good!
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 | |
Comment Actions
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.
Comment Actions
FWIW, I haven't reviewed fuse_xattrlist_convert at all because it looks "complicated" and still have hesitations about doing it in-place.
| 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? | |
Comment Actions
- remove extattr namespaces sysctl.
- revet err check according style(9)
- swap printf() to debug_printf()
| sys/fs/fuse/fuse_vnops.c | ||
|---|---|---|
| 2178 | Please, fill free to provide any alternative implementation. I ready to review and test it. | |
Comment Actions
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 :).
Comment Actions
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.
Comment Actions
My suggestion is to use separate input and output buffers. Just removes some things that can go wrong rewriting in place.
Ok.
Comment Actions
- Swap inplace names conversion to malloced.
- Reorder names conversion function arguments.
- Fix style.
Comment Actions
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. | |
Comment Actions
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.