Re: [SCM] Samba Shared Repository - branch master updated

[Volker Lendecke]

On Wed, May 30, 2012 at 02:36:03AM +0200, Kai Blin wrote:
> The branch, master has been updated
>        via  aa01908 libcli/dns: Rename UDP-based calls to reflect their use
>        via  42e1b94 Add myself as libcli/dns maintainer
>        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
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 };

in one line. I know we have tons of uses of those, but I
would like to start a discussion about banning them.

With best regards,

Volker Lendecke

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: [phone], fax: [phone]
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

[David Disseldorp-2]

On Wed, 30 May 2012 07:59:22 +0200
Volker Lendecke <[hidden email]> wrote:

> 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 };
>
> in one line. I know we have tons of uses of those, but I
> would like to start a discussion about banning them.

+1 from me, macros that effect control flow are evil IMO.

My preference would be to use a two line if statement:
if (ptr == NULL)
         return WERR_NOMEM;

Cheers, David

[Volker Lendecke]

On Wed, May 30, 2012 at 11:25:59AM +0200, David Disseldorp wrote:

> On Wed, 30 May 2012 07:59:22 +0200
> Volker Lendecke <[hidden email]> wrote:
>
> > 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 };
> >
> > in one line. I know we have tons of uses of those, but I
> > would like to start a discussion about banning them.
>
> +1 from me, macros that effect control flow are evil IMO.
>
> My preference would be to use a two line if statement:
> if (ptr == NULL)
> return WERR_NOMEM;

If nobody objects, I will push the attached patch by the end
of this week.

With best regards,

Volker Lendecke

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: [phone], fax: [phone]
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

[Kai Blin-4]

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.

> 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.

Cheers,
Kai

--
Kai Blin
Worldforge developer http://www.worldforge.org/
Wine developer http://wiki.winehq.org/KaiBlin
Samba team member http://www.samba.org/samba/team/

[Michael Adam-3]

Volker Lendecke wrote:
> On Wed, May 30, 2012 at 02:36:03AM +0200, Kai Blin wrote:
> > The branch, master has been updated
> >        via  aa01908 libcli/dns: Rename UDP-based calls to reflect their use
> >        via  42e1b94 Add myself as libcli/dns maintainer
> >        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?

As a mathematician, you know that the fact that someone uses this
macro, does not imply that we have a rule that we must use then.... 8-}

(even if someone changes existing code to use them - maybe he likes them)

> 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.

That is is a good argument.
Having used these macros myself in the past, I would agree to ban them.

> To save precious screen space, I would rather go and
> introduce a special rule to say
>
> if (ptr == NULL) { return WERR_NOMEM };

-1

Let's not discuss about some lines of code less.

Michael

[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

[Michael Adam-3]

Volker Lendecke wrote:

> On Wed, May 30, 2012 at 11:25:59AM +0200, David Disseldorp wrote:
> > On Wed, 30 May 2012 07:59:22 +0200
> > Volker Lendecke <[hidden email]> wrote:
> >
> > > 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 };
> > >
> > > in one line. I know we have tons of uses of those, but I
> > > would like to start a discussion about banning them.
> >
> > +1 from me, macros that effect control flow are evil IMO.
> >
> > My preference would be to use a two line if statement:
> > if (ptr == NULL)
> > return WERR_NOMEM;

Uh, for me this is even far worse than the one-line statment
above. IMHO We should not do this kind of indentation without
braces. It is too error-prone. Too easy to mis-read.

> If nobody objects, I will push the attached patch by the end
> of this week.

OK for me.

Cheers - Michael

> >From 3e53b8da6f1a0b56b1d32afdb7e77b173ce8ef14 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <[hidden email]>
> Date: Wed, 30 May 2012 10:14:51 +0200
> Subject: [PATCH] Coding: Add comment disproving control-flow changing macros
>
> ---
>  README.Coding |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/README.Coding b/README.Coding
> index 8416290..956a733 100644
> --- a/README.Coding
> +++ b/README.Coding
> @@ -367,3 +367,13 @@ Bad Example:
>   ret = some_function_my_name(get_some_name());
>   ...
>  
> +Control-Flow changing macros
> +----------------------------
> +
> +Macros like NT_STATUS_NOT_OK_RETURN that change control flow
> +(return/goto/etc) from within the macro are considered bad, because
> +they look like function calls that never change control flow. Please
> +do not use them in new code.
> +
> +The only exception is the test code that depends repeated use of calls
> +like CHECK_STATUS, CHECK_VAL and others.
> --
> 1.7.8

[Michael Adam-3]

Andrew Bartlett wrote:

> 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.

Some additional thoughts.

It is true that the code using the macros is more compact,
and hence possibly easier to read and grasp at first sight.
I also liked the macros quite a lot. But while it might be
easier to start off with a better error handling, this
might also be deceiving safety in some situations, since
the automatic use of these macros might make you forget
some special error handling (freeing of memory, whatnot)
that needs to be done. So currently I tend to agree with
Volker that the values in the compactness of code do not
make up for the additional sublte dangers and possible
misunderstandings in the code.

Cheers - Michael

[simo]

On Wed, 2012-05-30 at 12:17 +0200, Volker Lendecke wrote:

> On Wed, May 30, 2012 at 11:25:59AM +0200, David Disseldorp wrote:
> > On Wed, 30 May 2012 07:59:22 +0200
> > Volker Lendecke <[hidden email]> wrote:
> >
> > > 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 };
> > >
> > > in one line. I know we have tons of uses of those, but I
> > > would like to start a discussion about banning them.
> >
> > +1 from me, macros that effect control flow are evil IMO.
> >
> > My preference would be to use a two line if statement:
> > if (ptr == NULL)
> > return WERR_NOMEM;
>
> If nobody objects, I will push the attached patch by the end
> of this week.

ACK

Simo.


--
Simo Sorce
Samba Team GPL Compliance Officer <[hidden email]>
Principal Software Engineer at Red Hat, Inc. <[hidden email]>

[David Disseldorp-2]

On Wed, 30 May 2012 12:17:59 +0200
Volker Lendecke <[hidden email]> wrote:

> If nobody objects, I will push the attached patch by the end
> of this week.

Looks good.

Cheers, David

[simo]

On Wed, 2012-05-30 at 21:25 +1000, Andrew Bartlett wrote:

> 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.

I found that this is true only after the first pass, in time when
changes pile up this kind of code tend to introduce horrible leeks when
not outright flow errors.

But the worst thing is that it kills readability.
goto and return keywords are normally highlighted in text editors that
can do syntax highlighting, making it easy to spot them. Not so with
macros.

> > > 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).

Torture/Test code can be exempted, as it is a special purpose piece of
code, and we do not care as much about leaks and other resource
mishandling that is important long running process errors there.

> 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).

This is bad, in fact reading samba code these days is quite painful. Not
as bad as some projects, but bad. Syntax consistency helps the brain
concentrate on the important things.

> 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.

I am with Volker here. 'No surprises' beats 'slightly more convenient at
the time of writing' 10 - 1, for the simple reason that code is read
many more times than it is written. It is important for the code to be
readable, it is much more important than slight conveniences when you
write it.

Simo.

--
Simo Sorce
Samba Team GPL Compliance Officer <[hidden email]>
Principal Software Engineer at Red Hat, Inc. <[hidden email]>

[Andrew Bartlett]

On Wed, 2012-05-30 at 13:32 +0200, Michael Adam wrote:

> Andrew Bartlett wrote:
> > 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.
>
> Some additional thoughts.
>
> It is true that the code using the macros is more compact,
> and hence possibly easier to read and grasp at first sight.
> I also liked the macros quite a lot. But while it might be
> easier to start off with a better error handling, this
> might also be deceiving safety in some situations, since
> the automatic use of these macros might make you forget
> some special error handling (freeing of memory, whatnot)
> that needs to be done. So currently I tend to agree with
> Volker that the values in the compactness of code do not
> make up for the additional sublte dangers and possible
> misunderstandings in the code.

One of the great problems is the great power of a macro like:

#define NT_STATUS_HAVE_NO_MEMORY_AND_FREE(x, ctx) do { \
        if (!(x)) {\
                talloc_free(ctx); \
                return NT_STATUS_NO_MEMORY;\
        }\
} while (0)

#define NT_STATUS_NOT_OK_RETURN_AND_FREE(x, ctx) do { \
        if (!NT_STATUS_IS_OK(x)) {\
                talloc_free(ctx); \
                return x;\
        }\
} while (0)

These 'simple' macros do a lot, but also save a lot of repeated code
that we tend to get wrong.  But it is easy to look down a page of code
and confirm that all the error handling macros look the same.

Perhaps this is why I enjoy python and exceptions...

Andrew Bartlett

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

[Stefan (metze) Metzmacher]

Hi Volker,

> If nobody objects, I will push the attached patch by the end
> of this week.

Looks good to me. I already try to avoid them in new code.

metze

[Kai Blin-4]

On 2012-05-30 12:17, Volker Lendecke wrote:

> If nobody objects, I will push the attached patch by the end
> of this week.

I'll object. I don't think only providing anti-patterns is what we want
to go for for README.Coding. Don't just tell people what not to do, tell
them what to do instead. The rest of the file always has "Good" and
"Bad" examples.

As Simo mentioned, consistency is important, so please if you don't like
the macro, provide a canonical replacement for it.

Cheers,
Kai

--
Kai Blin
Worldforge developer http://www.worldforge.org/
Wine developer http://wiki.winehq.org/KaiBlin
Samba team member http://www.samba.org/samba/team/

[Volker Lendecke]

On Wed, May 30, 2012 at 02:29:13PM +0200, Kai Blin wrote:
> On 2012-05-30 12:17, Volker Lendecke wrote:
>
> > If nobody objects, I will push the attached patch by the end
> > of this week.
>
> I'll object.

Ok. I'll not push it.

> I don't think only providing anti-patterns is what we want
> to go for for README.Coding. Don't just tell people what not to do, tell
> them what to do instead. The rest of the file always has "Good" and
> "Bad" examples.
>
> As Simo mentioned, consistency is important, so please if you don't like
> the macro, provide a canonical replacement for it.

The problem is that there is no canonical replacement, as
the variety of macros is so large. I can certainly go
through all the macros and expand them manually, but this
from my point of view does not provide any additional
information beyond the "please don't use control-flow
changing macros".

Sorry for not being able to provide an appropriate wording.
Probably this is just too complex a topic to be put into
README.Coding, and we will have to live with people using
and people not using them.

With best regards,

Volker Lendecke

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: [phone], fax: [phone]
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

[David Disseldorp-2]

On Wed, 30 May 2012 14:43:10 +0200
Volker Lendecke <[hidden email]> wrote:

> The problem is that there is no canonical replacement, as
> the variety of macros is so large. I can certainly go
> through all the macros and expand them manually, but this
> from my point of view does not provide any additional
> information beyond the "please don't use control-flow
> changing macros".

Perhaps we could add something similar to the relevant section in the
Linux kernel CodingStyle[1] doc:

<quote>
Macros with multiple statements should be enclosed in a do - while block:

#define macrofun(a, b, c) \
        do { \
                if (a == 5) \
                        do_this(b, c); \
        } while (0)

Things to avoid when using macros:

1) macros that affect control flow:

#define FOO(x) \
        do { \
                if (blah(x) < 0) \
                        return -EBUGGERED; \
        } while(0)

is a _very_ bad idea.  It looks like a function call but exits the "calling"
function; don't break the internal parsers of those who will read the code.

2) macros that depend on having a local variable with a magic name:

#define FOO(val) bar(index, val)
</quote>

Cheers, David

1. http://www.kernel.org/doc/Documentation/CodingStyle

[David Disseldorp-2]

On Wed, 30 May 2012 14:58:40 +0200
David Disseldorp <[hidden email]> wrote:

> Perhaps we could add something similar to the relevant section in the
> Linux kernel CodingStyle[1] doc:

See patch attached, it includes Volker's prior text alongside some
examples from the kernel CodingStyle doc.

Cheers, David

[Kai Blin-4]

On 2012-05-30 15:16, David Disseldorp wrote:

> See patch attached, it includes Volker's prior text alongside some
> examples from the kernel CodingStyle doc.

Thanks, that helps to understand what's meant by the guideline.

I still disagree with the claim that the fact a macro called
"NT_STATUS_NOT_OK_RETURN" changes control flow is a surprise, but if the
rest of the team prefers to spell out the error handling every time,
then that's what we'll have to do. I'll just look into an editor macro
instead of a C preprocessor macro to reduce my typing then. :)

Cheers,
Kai

--
Kai Blin
Worldforge developer http://www.worldforge.org/
Wine developer http://wiki.winehq.org/KaiBlin
Samba team member http://www.samba.org/samba/team/

[Volker Lendecke]

On Wed, May 30, 2012 at 03:16:50PM +0200, David Disseldorp wrote:
> On Wed, 30 May 2012 14:58:40 +0200
> David Disseldorp <[hidden email]> wrote:
>
> > Perhaps we could add something similar to the relevant section in the
> > Linux kernel CodingStyle[1] doc:
>
> See patch attached, it includes Volker's prior text alongside some
> examples from the kernel CodingStyle doc.

I like that piece, but as there is significant disagreement
we should just leave it as it is. I should have known better
that coding style discussions can never reach consensus.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: [phone], fax: [phone]
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

[Andrew Bartlett]

On Wed, 2012-05-30 at 15:26 +0200, Kai Blin wrote:

> On 2012-05-30 15:16, David Disseldorp wrote:
>
> > See patch attached, it includes Volker's prior text alongside some
> > examples from the kernel CodingStyle doc.
>
> Thanks, that helps to understand what's meant by the guideline.
>
> I still disagree with the claim that the fact a macro called
> "NT_STATUS_NOT_OK_RETURN" changes control flow is a surprise, but if the
> rest of the team prefers to spell out the error handling every time,
> then that's what we'll have to do. I'll just look into an editor macro
> instead of a C preprocessor macro to reduce my typing then. :)

On this I certainly agree.  Some of these macros say what they are
doing, and others do not.  NT_STATUS_HAVE_NO_MEMORY doesn't say 'btw, I
return NT_STATUS_NO_MEMORY here' very well.

Andrew Bartlett

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