abee79f vfs: Use posix_sys_acl_blob_get_file in vfs_gpfs for posix ACLs

Andrew Bartlett abartlet at samba.org
Thu Oct 11 23:04:31 MDT 2012


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 Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org




More information about the samba-technical mailing list