Bug 8191 - vfs_gpfs dosn't honor ACE_FLAG_INHERITED_ACE
Summary: vfs_gpfs dosn't honor ACE_FLAG_INHERITED_ACE
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 3.6.0rc1
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-31 14:04 UTC by Gregor Beck (550 Unknown user)
Modified: 2012-05-23 20:40 UTC (History)
1 user (show)

See Also:

Attachments
patchset for v3-6-test (3.00 KB, patch)
2011-05-31 14:15 UTC, Gregor Beck (550 Unknown user) 		obnox: review+
jra: review+
Details
Jeremy's add-on patch (4.93 KB, patch)
2011-06-01 08:14 UTC, Michael Adam 		obnox: review+
jra: review+
Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gregor Beck (550 Unknown user) 2011-05-31 14:04:18 UTC 
vfs_gpfs masks out the flag ACE_FLAG_INHERITED_ACE supported by gpfs
Comment 1 Gregor Beck (550 Unknown user) 2011-05-31 14:15:56 UTC 
Created attachment 6503 [details]
patchset for v3-6-test
Comment 2 Jeremy Allison 2011-05-31 20:12:31 UTC 
Comment on attachment 6503 [details]
patchset for v3-6-test

I *hate* this patch (sorry :-). I'm changing it for master and will add an updated version for 3.6.0.

Directly manipulating bits in the ACE mask using a mix of SEC_ACE_XXX and SMB_ACE4_XXX flags is a recipe for disaster later on.

I'm factoring this out to 2 functions which make the bit manipulations clearer.

Jeremy.
Comment 3 Michael Adam 2011-05-31 20:14:40 UTC 
Comment on attachment 6503 [details]
patchset for v3-6-test

ACK
Comment 4 Jeremy Allison 2011-05-31 20:15:47 UTC 
Sorry nack on this patch as-is. Please wait for my 3.6.0 update.

Jeremy.
Comment 5 Michael Adam 2011-05-31 20:24:31 UTC 
Comment on attachment 6503 [details]
patchset for v3-6-test

sorry, jeremy, I missed your comment
Comment 6 Michael Adam 2011-05-31 20:39:30 UTC 
(In reply to comment #2)
> Comment on attachment 6503 [details]
> patchset for v3-6-test
> 
> I *hate* this patch (sorry :-). I'm changing it for master and will add an
> updated version for 3.6.0.
> 
> Directly manipulating bits in the ACE mask using a mix of SEC_ACE_XXX and
> SMB_ACE4_XXX flags is a recipe for disaster later on.

I don't really understand your objection here:
There is no mix of SEC_ACE and SMB_ACE4 flags used directly.
It is just a mapping from SEC_ACE to SMB_ACE4 and vice versa.

I do agree that for the sake of clarity, it might be useful to
create mapping functions for these two directions.

Cheers - Michael
 
> I'm factoring this out to 2 functions which make the bit manipulations clearer.
Comment 7 Michael Adam 2011-05-31 21:08:33 UTC 
Summing up an irc discussion:

* Gregor's patches are ok, but the original mapping code is hacky,
  since it simpy assigns SMB_ACE4 flags to SEC_ACE flags (and vice
  versa), which works by coincidence since the old supported flags
  were 1:1 bit-wise.

* Jeremy is working on an add-on patch that will provide proper
  mapping functions for the ace flag bits.
  This patch will be added as a second part of the bugfix.

Cheers - Michael
Comment 8 Michael Adam 2011-05-31 21:09:40 UTC 
Comment on attachment 6503 [details]
patchset for v3-6-test

re-ACKing this first patch after our discussion. Adding Jeremy as second reviewer. And waiting for Jeremy's add-on patch.
Comment 9 Michael Adam 2011-05-31 21:10:56 UTC 
the review flags confuse me
Comment 10 Jeremy Allison 2011-05-31 23:54:32 UTC 
Waiting for autobuild....
Comment 11 Michael Adam 2011-06-01 08:14:16 UTC 
Created attachment 6507 [details]
Jeremy's add-on patch

This is Jeremy's add-on patch that has landed in master.
I would like to have this in 3.6 as well, since it greatly
improves the code.
Comment 12 Michael Adam 2011-06-01 08:15:14 UTC 
Jeremy, could you review (and ACK) both patches, and hand the bug over to Karolin? Thanks!
Comment 13 Michael Adam 2011-06-01 08:16:05 UTC 
Comment on attachment 6507 [details]
Jeremy's add-on patch

Requesting review from jeremy for inclusion into 3.6.0

btw. this patch applies on top of the first patchset.
Comment 14 Jeremy Allison 2011-06-01 22:04:10 UTC 
Re-assigning to Karolin to add both to 3.6.0.
Jeremy.
Comment 15 Karolin Seeger 2011-06-02 18:53:44 UTC 
Pushed to v3-6-test.
Closing out bug report.

Thanks!