Bug 8419 - Make VFS op "streaminfo" stackable
Summary: Make VFS op "streaminfo" stackable
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 3.6.0
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-29 13:36 UTC by franklahm
Modified: 2011-10-20 17:54 UTC (History)
0 users

See Also:


Attachments
Patch (2.78 KB, text/plain)
2011-08-29 13:36 UTC, franklahm
no flags Details
Fixed patch (2.28 KB, patch)
2011-08-29 13:41 UTC, franklahm
no flags Details
Patch implementing function wrapper vfs_streaminfo (5.79 KB, patch)
2011-08-30 08:39 UTC, franklahm
no flags Details
Patch implementing function wrapper vfs_streaminfo (5.66 KB, patch)
2011-08-31 09:26 UTC, franklahm
no flags Details
Patch adding support for VFS op streaminfo chaining in all relevant VFS modules (5.37 KB, patch)
2011-09-21 08:09 UTC, franklahm
no flags Details
git-am fix I pushed to master (8.72 KB, patch)
2011-10-14 18:33 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.1 (6.42 KB, patch)
2011-10-17 20:06 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.2 containing both changes. (15.22 KB, patch)
2011-10-19 17:21 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description franklahm 2011-08-29 13:36:47 UTC
Created attachment 6824 [details]
Patch

Currently the VFS op "streaminfo" is not stackable, ie it's only
possible to have one active VFS module to take care of streams, eg
"streams_xattr".
Now what I'm trying to do is enhance the Netatalk VFS module, so that
it presents the Netatalk AppleDouble stuff as streams. But as I only
want to implement dealing with the relevant streams in the Netatalk
VFS mod, leaving all the rest to the "default" stream module
(streams_xattr), I need to
a) place the Netatalk VFS mod before streams_xattr
b) patch the VFS base code and modules to make the streaminfo op stackable

Patch attached.
Comment 1 franklahm 2011-08-29 13:41:19 UTC
Created attachment 6825 [details]
Fixed patch

Previous patch contained changes for the modules itself.
Comment 2 Volker Lendecke 2011-08-29 14:06:37 UTC
Please leave the macro SMB_VFS_STREAMINFO untouched. As the smb_vfs_call routines, the macros are supposed to be completely boiler-plate.

Please create a wrapper function and change all the callers.

If you insist on changing the macros, please get other developers to overrule me here via samba-technical@samba.org.

With best regards,

Volker Lendecke
Comment 3 franklahm 2011-08-30 08:39:47 UTC
Created attachment 6838 [details]
Patch implementing function wrapper vfs_streaminfo
Comment 4 Volker Lendecke 2011-08-30 13:26:42 UTC
(In reply to comment #3)
> Created attachment 6838 [details]
> Patch implementing function wrapper vfs_streaminfo

Can you replace

+	VFS_FIND(streaminfo);
+	return handle->fns->streaminfo(handle, fsp, fname, mem_ctx,
+				       num_streams, streams);

by
+       return SMB_VFS_STREAMINFO(...)

?

Volker
Comment 5 franklahm 2011-08-31 09:26:53 UTC
Created attachment 6843 [details]
Patch implementing function wrapper vfs_streaminfo

Replace

+    VFS_FIND(streaminfo);
+    return handle->fns->streaminfo(handle, fsp, fname, mem_ctx,
+                       num_streams, streams);

by

+       return SMB_VFS_STREAMINFO(...)
Comment 6 franklahm 2011-09-21 08:09:04 UTC
Created attachment 6924 [details]
Patch adding support for VFS op streaminfo chaining in all relevant VFS modules

Patch adding support for VFS op streaminfo chaining in all relevant VFS modules.
Comment 7 Jeremy Allison 2011-10-13 18:39:31 UTC
The problem with the patch is that it doesn't initialize the variables when SMB_VFS_STREAMINFO() is called - see delete_all_streams() for example.

It needs to also initialize *stream_info and num_streams to NULL, 0, before calling SMB_VFS_STREAMINFO() otherwise the stacked modules are reallocating uninitialized memory.

I'll fix this up and post for your review.

Jeremy.
Comment 8 Jeremy Allison 2011-10-13 22:43:13 UTC
I've pushed a fixed patch to master. Once it passes autobuild I'll update here for your review.

Jeremy.
Comment 9 Jeremy Allison 2011-10-14 18:33:47 UTC
Created attachment 7002 [details]
git-am fix I pushed to master

Frank, here is what I've pushed to master. Can you test this in 3.6.x and report back if it works for you. If so I'll get a second Team review and we'll get it added to 3.6.x.

Cheers,

Jeremy.
Comment 10 franklahm 2011-10-15 10:00:06 UTC
(In reply to comment #7)
> The problem with the patch is that it doesn't initialize the variables when
> SMB_VFS_STREAMINFO() is called - see delete_all_streams() for example.

Did you possibly missed [patch 1/2] ?
<https://attachments.samba.org/attachment.cgi?id=6843>

That's the one implementing the new function wrapper vfs_streaminfo() which does the necessary initialisation and replaces SMB_VFS_STREAMINFO().

Afaict you only reviewed, fixed and commited [patch 2/2]:
<https://attachments.samba.org/attachment.cgi?id=6924>

Thanks!
-f
Comment 11 Jeremy Allison 2011-10-17 16:50:12 UTC
Ah yes - that's the problem when you split a git-am patch into two attachments. I missed the first one (sorry).

The way to do this is to type:

git format-patch --stdout REFSPEC1..REFSPEC2 >/tmp/patch-name

- that puts both fixes into one file.

I'll add in your patch #1 as well.

Cheers,

Jeremy.
Comment 12 Jeremy Allison 2011-10-17 20:06:55 UTC
Created attachment 7006 [details]
git-am fix for 3.6.1

Second part of fix. Frank, please review with both and I'll get a Team review to add for 3.6.2.

Thanks,

Jeremy.
Comment 13 franklahm 2011-10-19 09:39:15 UTC
Comment on attachment 7002 [details]
git-am fix I pushed to master

This one should be marked obsolete.
Comment 14 franklahm 2011-10-19 09:42:46 UTC
(In reply to comment #12)
> Created attachment 7006 [details]
> git-am fix for 3.6.1
> 
> Second part of fix. Frank, please review with both and I'll get a Team review
> to add for 3.6.2.

Thanks a lot. I've tested your patch 7006 together with my patch 6924.  Looks good. 7002 should be marked obsolete.
As you've applied 7002 to master, you've got to either revert that or adjust 6924/7006 accordingly, but I guess you're aware of that.
Sorry for the mess I introduced by submitting the patches seperately.

Thanks for you help!
Comment 15 Jeremy Allison 2011-10-19 16:59:45 UTC
Comment on attachment 7002 [details]
git-am fix I pushed to master

No, this one isn't obsolete as it does more than just set the variables to NULL.

It's safer to have both fixes in, so I'm going to leave it like that.
Comment 16 Jeremy Allison 2011-10-19 17:21:15 UTC
Created attachment 7011 [details]
git-am fix for 3.6.2 containing both changes.

Final fix for 3.6.2. Volker please review.

Thanks !

Jeremy.
Comment 17 Jeremy Allison 2011-10-19 20:59:09 UTC
Re-assigning to Karolin for inclusion in 3.6.2.

Jeremy.
Comment 18 Karolin Seeger 2011-10-20 17:54:03 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!