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.
Note that 2) is not triggered by delete_all_streams(), but by a server side copy request.
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 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.
(In reply to Jeremy Allison from comment #3) metze likes cmp, but I can change that. :) Thanks!
(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.
Created attachment 11499 [details] Patch for master Patch reworked as suggested by Jeremy.
Comment on attachment 11499 [details] Patch for master LGTM. Pushed to master !
Created attachment 11741 [details] Patch for 4.2 and 4.3, cherry-picked from master
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.
Reassigning to Karolin for inclusion in 4.3.next, 4.2.next.
Pushed to autobuild-v4-[2|3]-test.
(In reply to Karolin Seeger from comment #11) Pushed to both branches. Closing out bug report. Thanks!