Bug 11466 - Copy files with vfs_fruit fails when using vfs_streams_xattr without stream prefix and type suffix
Summary: Copy files with vfs_fruit fails when using vfs_streams_xattr without stream p...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-24 16:39 UTC by Ralph Böhme
Modified: 2021-01-18 11:15 UTC (History)
2 users (show)

See Also:


Attachments
Patch for master (6.54 KB, patch)
2015-08-25 08:50 UTC, Ralph Böhme
no flags Details
Patch for master (6.50 KB, patch)
2015-10-16 11:00 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.2 and 4.3, cherry-picked from master (6.95 KB, patch)
2015-12-23 21:41 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2015-08-24 16:39:00 UTC
Given the following vfs_fruit share:

[Global]
    fruit:copyfile = yes

[fruit]
    path = /Volumes/fruit
    vfs objects = catia fruit streams_xattr
#    fruit:metadata = netatalk (= default)
    streams_xattr:prefix = user.
    streams_xattr:store_stream_type = no

trying to copy files with a Netatalk metadata xattr fails, because the Finder creates a temp file, vfs_fruit attaches a Netatalk metadata xattr to it, then the Finder deletes the temp file via setinfo(delete_on_close=1).

This calls delete_all_streams() which uncovers two issues:

1.
In the deletion loop vfs_streaminfo is called which will return two streams for the Netatalk metadata xattr: ":AFP_AfpInfo:$DATA" and ":org.netatalk.Metadata:$DATA".
Both are basically aliases to the same xattr, so once SMB_VFS_UNLINK is called with one of them, the subsequent deletion will fail. In order to fix this, we must filter out the Netatalk metadata xattr from streaminfo in vfs_fruit.

2.
If fruit:copyfile is enabled, after finishing the copy of the file we end up in fruit_copy_chunk_done() where we'll copy all file streams as well. First we get a list of stream names from vfs_streaminfo and then we loop over the stream list calling copy_file() for each stream.
As the stream name will always have a stream type, the mapping of stream name to xattr name in vfs_streams_xattr will fail, because the stream typte ":$DATA" is carried over to the xattr name.
Comment 1 Ralph Böhme 2015-08-25 08:46:42 UTC
Note that 2) is not triggered by delete_all_streams(), but by a server side copy request.
Comment 2 Ralph Böhme 2015-08-25 08:50:23 UTC
Created attachment 11365 [details]
Patch for master

Hey metze, can you please take a look and comment on whether we may actually want to split this into two bugreports and if we want tests?
Comment 3 Jeremy Allison 2015-08-25 23:51:38 UTC
Comment on attachment 11365 [details]
Patch for master

In part #2 of this patch:

+       for (i = 0; i < *num_streams; i++) {
+               cmp = strcasecmp_m(tmp[i].name, name);
+               if (cmp != 0) {
+                       continue;
+               }
+               break;
+       }
+
+       if (cmp != 0) {
+               return true;
+       }


Instead of cmp = strcasecmp_m(tmp[i].name, name), use:

if (strequal_m(mp[i].name, name)) {
      break;
}

...

if (i == *num_streams) {
      return true;
}

That way you don't need the 'cmp' variable.
Comment 4 Ralph Böhme 2015-08-26 15:27:56 UTC
(In reply to Jeremy Allison from comment #3)
metze likes cmp, but I can change that. :)
Thanks!
Comment 5 Jeremy Allison 2015-08-26 15:54:26 UTC
(In reply to Ralph Böhme from comment #4)

Yeah, but sometimes metze's taste can be different :-). I personally only add variables for extra clarity, and I don't think that's needed here.
Comment 6 Ralph Böhme 2015-10-16 11:00:15 UTC
Created attachment 11499 [details]
Patch for master

Patch reworked as suggested by Jeremy.
Comment 7 Jeremy Allison 2015-10-16 18:23:42 UTC
Comment on attachment 11499 [details]
Patch for master

LGTM. Pushed to master !
Comment 8 Ralph Böhme 2015-12-23 21:41:22 UTC
Created attachment 11741 [details]
Patch for 4.2 and 4.3, cherry-picked from master
Comment 9 Jeremy Allison 2015-12-24 01:02:02 UTC
Comment on attachment 11741 [details]
Patch for 4.2 and 4.3, cherry-picked from master

LGTM.

However, there is one tidy-up I think is worth making (in master only) which I missed in master review.

The first patch has:

+static bool del_fruit_stream(TALLOC_CTX *mem_ctx, unsigned int *num_streams,
+			     struct stream_struct **streams,
+			     const char *name)
+{
+	struct stream_struct *tmp = *streams;
+	int i;
+
+	if (*num_streams == 0) {
+		 return true;
+	 }
+
+	for (i = 0; i < *num_streams; i++) {
+		if (strequal_m(tmp[i].name, name)) {
+			break;
+		}
+	}
+
+	if (i == *num_streams) {
+		return true;
+	}

The initial:

if (*num_streams == 0) {
        return true;
}

is redundant, as if *num_streams == 0 then the for loop immediately terminates and the clause
if (i == *num_streams) will catch the same case.

Removing it (in master) just looks tidier to me.
Comment 10 Jeremy Allison 2015-12-24 01:02:56 UTC
Reassigning to Karolin for inclusion in 4.3.next, 4.2.next.
Comment 11 Karolin Seeger 2016-01-05 08:44:16 UTC
Pushed to autobuild-v4-[2|3]-test.
Comment 12 Karolin Seeger 2016-01-07 17:33:25 UTC
(In reply to Karolin Seeger from comment #11)
Pushed to both branches.
Closing out bug report.

Thanks!