abee79f vfs: Use posix_sys_acl_blob_get_file in vfs_gpfs for posix ACLs

[simo]

On Fri, 2012-10-12 at 16:04 +1100, Andrew Bartlett wrote:
> On Thu, 2012-10-11 at 12:32 -0400, simo wrote:
> > On Thu, 2012-10-11 at 09:06 -0700, Jeremy Allison wrote:
> > > On Thu, Oct 11, 2012 at 01:32:27PM +0200, Volker Lendecke wrote:
> > > > On Thu, Oct 11, 2012 at 10:16:04PM +1100, Andrew Bartlett wrote:
> > > > > On Thu, 2012-10-11 at 13:02 +0200, Christian Ambach wrote:
> > > > > > Andrew,
> > > > > 
> > > > > > I do not really understand what the 
> > > > > > expected behavior of of gpfsacl_sys_acl_blob_get_fd() and 
> > > > > > gpfsacl_sys_acl_blob_get_fd() should be. GPFS supports both the posix 
> > > > > > and NFSv4 ACL model, so I guess it will take special care that both 
> > > > > > cases are treated correctly.
> > > > > 
> > > > > The idea is simply:
> > > > > 
> > > > > If there is an NFSv4 ACL, then we give an error.  The layers above this
> > > > > cope with errors, and when the patch to vfs_acl_common is applied
> > > > > (currently there are no callers) it will simply fall back to the hash of
> > > > > the NT ACL.  (And given the simple mapping, this is less important to
> > > > > implement). 
> > > > > 
> > > > > If it is a posix ACL, then call the posix helper function that
> > > > > linearlises the posix ACL into a blob, along with the owner, group and
> > > > > mode.
> > > > > 
> > > > > What was the error BTW?
> > > > 
> > > > You DID notice that you broke code, right? AGAIN?
> > > 
> > > Ok. I think it's time to revive Volker's original idea of reviewer-must-push
> > > for *all* changes, even for master.
> > > 
> > > The fault is not entirely Andrew's, both Simo and I missed this
> > > problem (along with the subsequent crash bug) in the review, so
> > > this means we really didn't do a good enough job on this review.
> > 
> > Well mine was just a first order review,
> > I asked for the style changes and break up to make the changes readable
> > *exactly* because the first version was a gargantuan patch with a lot of
> > repetitions and long lines.
> > 
> > My intention was to do a second review of the single patches once those
> > changes were made.
> 
> Simo,
> 
> I'm sorry, as I have clearly misunderstood your intent here.  I clearly
> missed your intent to perform a second review, and focused on the
> whitespace and formatting cleanups that your requested, and integrated
> those into the patches. 
> 
> I then got positive review from the ACL maintainer (Jeremy), and pushed
> the result. 
> 
> > Apparently though Andrew decided my request were unreasonable, applied a
> > very annoying 'indenting patch' I also asked not to push in that form on
> > IRC. 
> 
> The re-indentation was only over the skeleton and vfs_default modules,
> which are boilerplate with no substantive code or history on their own,
> but are (by copy and paste) the source of much poor style across the
> rest of the codebase.  I did this because the poor style was pervasive
> in those files, not just in the functions I changed.  (vfs_default still
> needs some work). 
> 
> The remaining formatting changes were made in the original patches that
> changed each module, as you specifically requested. 
> 
> > And then just merely pushed everything to master without waiting for a
> > second review.
> > I find this behavior disrespectful of the time and resources the other
> > team members put when they make patches and then seek feedback and fix
> > their patches based on that feedback.
> 
> For the disrespect you feel I have given you, I can only say that I'm
> sorry.  I did spend a substantial amount of time dealing with the
> formatting issues you raised, and was totally unaware that you were
> expecting to do a second pass.  

Andrew,
that's how reviews work:

A ask for a modification
B makes it or explains why he thinks the request is wrong
A acks/nacks

repeat until ack

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>