Created attachment 12401 [details] WIP patchset for master Bug #12028 changed the default ACL we return in vfs_acl_xattr|tdb for a file or directory that lacks a ACL xattr blob. Cf #12028 for the rationale behind that. Discussion on the mailing list [1] concluded that the changed behaviour is the right thing to do, but because it can cause problems for existing setups, we revert the default behaviour and put the modified behaviour behind an option. Patchset mostly ready, currently running private autobuilds and polishing commit messages, sneak preview attached. [1] <https://lists.samba.org/archive/samba/2016-August/202154.html>
Really nice cleanup (esp the config handle). In the WIP patch set - if the "windows" style is the right thing to do, why is the default "posix"? Thanks, Uri.
Comment on attachment 12401 [details] WIP patchset for master As we're commenting on the WIP :-). Can you change get_nt_acl_internal() in patch [PATCH 06/12] to do an early return (goto fail) if get_acl_blob returns an error status ? Then do status = validate_nt_acl_blob() at function scope, not inside the if (NT_STATUS_IS_OK(status)) { scope. That always looks a lot cleaner to me than nested function calls inside an if clause.
(In reply to Uri Simchoni from comment #1) As explained in comment #0 "...because it can cause problems for existing setups, we revert the default behaviour and put the modified behaviour behind an option.". But if others feel like making the "windows" style ACL the new default is the better decision, I'd concur. We put the change in WHATSNEW and affected users could switch to the previous behaviour.
(In reply to Jeremy Allison from comment #2) That won't work, because get_acl_blob() failing must be handled by continuing with fetch the sd from the filesystem in the following "if (psd == NULL) {...}" clause.
... [1929(13428)/1958 at 3h7m8s] samba4.drs.repl_move.python(promoted_dc)(promoted_dc) go, go, go! :)
(In reply to Ralph Böhme from comment #4) Fair enough, wasn't looking at it in full function context, just the diff. I'll wait until I see the finished product and shut up then :-).
Created attachment 12445 [details] Patch for 4.5 cherry-picked from master 4.3 and 4.4 still in the loop, backporting for those versions is a little more effort.
Created attachment 12447 [details] Patch for 4.4, backported from master
Created attachment 12448 [details] Patch for 4.3, backported from master
Reassigning to Karolin for inclusion in 4.5.0, 4.4.next, 4.3.next.
Pushed to v4-5-test.
Patch does not apply on current v4-4-test: user@host:/data/git/samba/v4-4-test$ git am v44-bug12177.patch Applying: Revert "vfs_acl_xattr: objects without NT ACL xattr" Applying: vfs_acl_common: rename psd to psd_blob in get_nt_acl_internal() Applying: vfs_acl_common: rename pdesc_next to psd_fs Applying: vfs_acl_common: remove redundant NULL assignment Applying: vfs_acl_common: simplify ACL logic, cleanup and talloc hierarchy Applying: vfs_acl_common: move the ACL blob validation to a helper function Applying: vfs_acl_tdb|xattr: use a config handle Applying: vfs_acl_common: move stat stuff to a helper function Applying: vfs_acl_common: check for ignore_system_acls before fetching filesystem ACL Applying: vfs_acl_xattr|tdb: add option to control default ACL style Applying: vfs_acl_common: Windows style default ACL Applying: s4/torture: tests for vfs_acl_xattr default ACL styles error: patch failed: selftest/target/Samba3.pm:1690 error: selftest/target/Samba3.pm: patch does not apply Patch failed at 0012 s4/torture: tests for vfs_acl_xattr default ACL styles
Pushed to autobuild-v4-3-test.
Re-assigning to Ralph, because the patch for 4.4 does not apply.
Created attachment 12466 [details] Patch for 4.4 backported from master
Re-assigning to Karolin for inclusion in 4.5.next, 4.4.next, 4.3.next.
(In reply to Jeremy Allison from comment #16) Pushed to v4-5-test.
Pushed to autobuild-v4-4-test.
Pushed to all branches. Closing out bug report. Thanks!