W_ERROR_HAVE_NO_MEMORY and friends

[Andrew Bartlett]

On Wed, 2012-05-30 at 12:35 +0200, Kai Blin wrote:
> On 2012-05-30 07:59, Volker Lendecke wrote:
> 
> >>        via  6a1ad76 s4-dns: Use W_ERROR_HAVE_NO_MEMORY in create_response_rr
> > 
> > Where is the rule laid down that we must use those macros?
> 
> I'm not aware of any rule, but the rest of the DNS server code is using
> that macro, so I thought consistency was a good thing.
> 
> > I find them completely obnoxious to use. Doing a return or
> > goto from something that looks like a function call is just
> > wrong to me. To save precious screen space, I would rather
> > go and introduce a special rule to say
> > 
> > if (ptr == NULL) { return WERR_NOMEM };
> 
> This is not about screen space for me, it's about having to write less
> boilerplate code. Code completion works better on the macro.

I've written code that uses it, and code that does not.  One of the
advantages is that it promotes efficient and correct error handling
(that is, checking every error, perhaps freeing a temp mem context). 

I tend to find that code that uses this has correct error handling,
while code that doesn't (and so has the manually expanded version) tends
to have poorer handling, often missing details. 

> > in one line. I know we have tons of uses of those, but I
> > would like to start a discussion about banning them.
> 
> If you want to ban them, go ahead. I don't feel strongly enough about
> this either way. As I said, my change was mostly to keep the
> allocation-error checks consistent, and it was easier to change this way
> around.

As the guideline eludes to, we do use this a lot in the torture code -
torture_assert() actually does a return, and this is the fundamental
basis of modern torture code.  (You can write torture_fail(), but this
is the current best practice). 

When I first saw the NT_STATUS_HAVE_NO_MEMORY macro, I didn't like it.
I certainly had the same concerns as Volker.  It does make the code more
compact, and tight code can be easier to grasp, but it's not a universal
rule. 

That said, I prefer it over the goto failed style. 

The one thing I would say is that our coding guidelines should reflect
the bulk of the recent, modern code we write and work with.  That is, I
honestly don't read our style guide often, but I'm very much influenced
by the style of the code I work on.  (Hence picking up
talloc_stackframe() in source3 code, HAVE_NO_MEMORY in code metze has
been involved with etc). 

At this point I am willing to be convinced either way, but I'll note the
alternative:   we could also have a style entry that says that these
(HAVE_NO and RETURN) macros are a common sight in Samba and these are
the semantics.  I certainly got used to them pretty quickly. 

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org