Bug 12490 - vfs_fruit correct Netatalk metadata xattr on FreeBSD
Summary: vfs_fruit correct Netatalk metadata xattr on FreeBSD
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.5.3
Hardware: x64 FreeBSD
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-31 14:15 UTC by Evan Champion
Modified: 2020-12-11 12:28 UTC (History)
5 users (show)

See Also:


Attachments
Patch for 4.6 cherry-picked from master (20.70 KB, patch)
2017-02-15 11:59 UTC, Ralph Böhme
jra: review+
Details
Patch for WHATSNEW (2.34 KB, patch)
2017-02-15 12:04 UTC, Ralph Böhme
jra: review+
metze: review+
kseeger: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Champion 2016-12-31 14:15:13 UTC
The netatalk compatibility xattr created by vfs_fruit has the wrong name on FreeBSD -- the correct netatalk metadata xattr is "org.netatalk.Metadata", but on FreeBSD vfs_fruit writes to "netatalk.Metadata".  This is because an #ifdef in vfs_fruit.c that determines the netatalk xattr names incorrectly identifies that FreeBSD does not require the "user." xattr namespace prefix.  The patch to correct this as well as a separate typo in a comment is below.  The patch is against samba 4.5.3.

--- vfs_fruit.c.orig	2016-12-31 15:32:26.585948000 +0800
+++ vfs_fruit.c	2016-12-31 15:33:05.071994000 +0800
@@ -105,7 +105,7 @@
  * This is hokey, but what else can we do?
  */
 #define NETATALK_META_XATTR "org.netatalk.Metadata"
-#if defined(HAVE_ATTROPEN) || defined(FREEBSD)
+#ifdef HAVE_ATTROPEN
 #define AFPINFO_EA_NETATALK NETATALK_META_XATTR
 #define AFPRESOURCE_EA_NETATALK "org.netatalk.ResourceFork"
 #else
@@ -3790,7 +3790,7 @@
 	}
 
 	/*
-	 * Now copy all reamining streams. We know the share supports
+	 * Now copy all remaining streams. We know the share supports
 	 * streams, because we're in vfs_fruit. We don't do this async
 	 * because streams are few and small.
 	 */


The results before and after applying the patch are:

File copied in with original vfs_fruit.c:
root@freebsd:/var/tmp/test # lsextattr user test
test	netatalk.Metadata

File copied in with patched vfs_fruit.c:
root@freebsd:/var/tmp/test # lsextattr user test
test	org.netatalk.Metadata

Comparison with file copied via netatalk3:
root@freebsd:/var/tmp/test # lsextattr user test
test	org.netatalk.Metadata


smb4.conf (share section only):
[test]
    path = /var/tmp/test
    printable = no
    writeable = yes
    browseable = yes
    vfs objects = zfsacl catia fruit streams_xattr aio_pthread
    hide dot files = yes
    guest ok = no
    nfs4:mode = special
    nfs4:acedup = merge
    nfs4:chown = true
    zfsacl:acesort = dontcare
    fruit:locking = none
    fruit:encoding = native
    streams_xattr:store_stream_type = no
    streams_xattr:prefix = user.

Thank you,

Evan
Comment 1 Ralph Böhme 2017-01-02 12:54:07 UTC
D'oh! It's not the FreeBSD xattr API that expects a namespace prefix (it takes a seperate int arg to denote the namesapce), but our libreplace xattr code in lib/replace/xattr.c expects a namespace prefix.

Now the problem is, fixing this breaks backward compatibility. Any existing system will loose access to exisiting metadata after updating. :/ As this affects the default fruit:metadata setting, we can't simply apply this patch, but must make this an option that default to the old, broken behaviour for 4.5 and older, making it the default for 4.6.

Does this sound reasonable? Did I miss anything?
Comment 2 Björn Jacke 2017-01-02 16:27:13 UTC
IMHO it would be a good idea to make the xattr namespace prefix configurable. I remember a proprietary filesystem on Linux which also had a different xattr namespace and the guys using it needed to patch samba accordingly. Shall we introduce a global option for that? Not sure if this might impact performance considerably also.
Comment 3 Evan Champion 2017-01-02 16:41:45 UTC
(In reply to Björn Jacke from comment #2)
In theory yes but in practice maybe not.  In this particular case the attribute needs to end up called exactly org.netatalk.Metadata for interoperability with netatalk.  In all the other uses for xattrs in samba (e.g. streams_xattr, DOSATTRIB) the most important thing is that the xattr is named consistently (and consistently between versions) and not necessarily correctly/as was expected.  Solaris for example doesn't need the "user." prefix but has been using "user." prefix and changing it now would seem to only cause problems.  streams_xattr already provides a way to change its prefix.
Comment 4 Ralph Böhme 2017-02-15 11:59:08 UTC
Created attachment 12935 [details]
Patch for 4.6 cherry-picked from master
Comment 5 Ralph Böhme 2017-02-15 12:04:47 UTC
Created attachment 12936 [details]
Patch for WHATSNEW

Here's the WHATSNEW text I prepared for this one.

Evan, I'm interested in your feedback as well. Does the proposed text cover the issue well enough from your perspective?
Comment 6 Jeremy Allison 2017-02-15 20:00:08 UTC
Re-assigning to Karolin for inclusion in 4.6.next.
Comment 7 Karolin Seeger 2017-02-17 10:45:58 UTC
Pushed to autobuild-v4-6-test.
Comment 8 Karolin Seeger 2017-02-21 08:47:09 UTC
(In reply to Karolin Seeger from comment #7)
Pushed to v4-6-test.
Closing out bug report.

Thanks!
Comment 9 Andriy Syrovenko 2017-07-16 14:34:23 UTC
This "fix" broke Samba as an AD DC on FreeBSD.
I've just file this as a new ticket here: https://bugzilla.samba.org/show_bug.cgi?id=12912