Removing the NT_STATUS_HAVE_NO_MEMORY_AND_FREE macro

[Andrew Bartlett]

On Mon, 2014-02-17 at 11:04 +0100, Volker Lendecke wrote:
> On Mon, Feb 17, 2014 at 01:58:19PM +1300, Andrew Bartlett wrote:
> > I've reviewed all of these, and uploaded them with review tags to 
> > https://gerrit.sernet.de/#/q/status:open+project:samba+branch:master
> > +topic:garming/macro-return,n,z
> > 
> > git://git.samba.org/abartlet/samba.git macro-return
> > 
> > Naturally, I don't propose to push them until Kamen's concerns over the
> > coding style itself are addressed or withdrawn. 
> 
> One other thing: In the past we have always avoided sweeping
> changes over the code for two reasons:
> 
> It clutters git blame and it makes backports harder than
> necessary.
> 
> I 100% agree we should try to follow what's in
> README.Coding, and I also 100% agree that control-flow
> changing macros are bad, but I value backport ease higher
> than hygiene for existing code.
> 
> So -1 from my side for these sweeping changes, sorry for
> that. This is just my opinion.

These changes (all the changes so far proposed) are not sweeping:

git diff origin/master..HEAD| diffstat
 auth/auth_sam_reply.c                      |   15 ++
 dfs_server/dfs_server_ad.c                 |   40 +++++--
 libcli/util/ntstatus.h                     |   34 -----
 libgpo/gpo_util.c                          |   40 +++++--
 source3/auth/auth_samba4.c                 |   10 +
 source3/auth/user_info.c                   |   55 +++++++--
 source3/passdb/pdb_samba_dsdb.c            |   30 ++++-
 source4/auth/sam.c                         |   92 +++++++++++-----
 source4/auth/session.c                     |   50 +++++++-
 source4/dsdb/common/util_groups.c          |    5 
 source4/dsdb/kcc/kcc_topology.c            |  143
+++++++++++++++++++------
 source4/dsdb/repl/drepl_out_helpers.c      |   20 ++-
 source4/ldap_server/ldap_extended.c        |    4 
 source4/lib/policy/gp_filesys.c            |   30 ++++-
 source4/lib/policy/gp_ldap.c               |  165
+++++++++++++++++++++++------
 source4/lib/policy/gp_manage.c             |   55 +++++++--
 source4/libnet/libnet_samsync_ldb.c        |   15 ++
 source4/ntvfs/posix/pvfs_acl.c             |   30 ++++-
 source4/rpc_server/lsa/dcesrv_lsa.c        |   40 +++++--
 source4/smbd/service_stream.c              |    5 
 source4/torture/util_smb.c                 |    5 
 source4/wrepl_server/wrepl_in_call.c       |   10 +
 source4/wrepl_server/wrepl_in_connection.c |    4 
 23 files changed, 682 insertions(+), 215 deletions(-)

If/when a patch is submitted that is sweeping, I could understand the
concern, but I think it would be better to document
NT_STATUS_HAVE_NO_MEMORY and NT_STATUS_NOT_OK_RETURN as exceptions in
README.Coding.  Our code should reflect our coding standards, otherwise
we get all the pain but none of the gain from having them.  That is,
coding standards should never be a case of 'do what I say, not what I
do'. 

As to Kamen's concerns regarding the 'copy' functions, I propose we
address them in two ways:

copy_netr_SamInfo3 and copy_netr_SamBaseInfo I think can be eliminated -
I've checked the consumers, and (essentially) a rework to instead call
talloc_move can do the same job.

If Kamen feels that the changes to gpo_copy() are bad enough, then I
propose that gpo.h be converted to IDL, and we deep copy the structure
with ndr_pull_struct_blob/ndr_push_struct_blob.

Finally, I would respectfully suggest that as we have the patches and
both Jeremy and I have reviewed some/all of the changes respectively,
the argument that we should be 'concentrating on real work' is a bit of
a red herring. 

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba