Re: [PATCH 4/5] altstack: Don't use 0 pointer literals

Hi David,

I'm back home in front of a real keyboard, and want to follow up with you
about your patches.  I must admit that my usual practice is to use NULL,
and not 0.  Around the time I wrote altstack I had recently read Jen
Gustedt's book, Modern C.  In it, he makes the point that NULL hides more
than it clarifies (see section 11.7).  While I find his argument a little
weak when considering a gcc environment where I know that NULL is defined
as ((void *)0), it seems sound when the target tool chain isn't known in
advance, as with a CCAN module.  Here's a link to the book and also a blog
article from Jens about NULL.  I'm very curious to know if you (or any
other CCAN contributors) find his arguments at all compelling.  Thanks.
 -Dan

http://icube-icps.unistra.fr/img_auth.php/d/db/ModernC.pdf
https://gustedt.wordpress.com/2010/11/07/dont-use-null/

On Fri, Jun 3, 2016 at 4:40 AM David Gibson <[email protected]>
wrote:

> In a number of places the altstack module uses a literal '0' for pointer
> values.  That's correct C, but doesn't make it obvious on a quick read
> whether values are integers or pointers.  This patch changes those cases
> to use the NULL define instead.
>
> Signed-off-by: David Gibson <[email protected]>
> ---
>  ccan/altstack/_info      |  4 ++--
>  ccan/altstack/altstack.c | 10 +++++-----
>  ccan/altstack/altstack.h |  6 +++---
>  ccan/altstack/test/run.c | 22 +++++++++++-----------
>  4 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/ccan/altstack/_info b/ccan/altstack/_info
> index 7accf86..b8cf6a8 100644
> --- a/ccan/altstack/_info
> +++ b/ccan/altstack/_info
> @@ -63,8 +63,8 @@
>   *             void *out;
>   *
>   *             assert(argc == 3);
> - *             stk_max = strtol(argv[1], 0, 0) * 1024 * 1024;
> - *             vla_sz  = strtol(argv[2], 0, 0) * 1024 * 1024;
> + *             stk_max = strtol(argv[1], NULL, 0) * 1024 * 1024;
> + *             vla_sz  = strtol(argv[2], NULL, 0) * 1024 * 1024;
>   *             assert(stk_max > 0 && vla_sz > 0);
>   *
>   *             snprintf(maps, sizeof(maps), "egrep '\\[stack'
> /proc/%d/maps", getpid());
> diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> index 6dfb9fa..2115740 100644
> --- a/ccan/altstack/altstack.c
> +++ b/ccan/altstack/altstack.c
> @@ -78,10 +78,10 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> *arg, void **out)
>
>         state.fn  = fn;
>         state.arg = arg;
> -       state.out = 0;
> +       state.out = NULL;
>         state.max = max;
>         state.ebuf[state.elen = 0] = '\0';
> -       if (out) *out = 0;
> +       if (out) *out = NULL;
>
>         // if the first page below the mapping is in use, we get max-pgsz
> usable bytes
>         // add pgsz to max to guarantee at least max usable bytes
> @@ -91,7 +91,7 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg,
> void **out)
>         ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { state.max,
> rl_save.rlim_max }), 1);
>         undo++;
>
> -       ok(m = mmap(0, max, PROT_READ|PROT_WRITE,
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_GROWSDOWN|MAP_NORESERVE, -1, 0), 1);
> +       ok(m = mmap(NULL, max, PROT_READ|PROT_WRITE,
> MAP_PRIVATE|MAP_ANONYMOUS|MAP_GROWSDOWN|MAP_NORESERVE, -1, 0), 1);
>         undo++;
>
>         if (setjmp(state.jmp) == 0) {
> @@ -130,9 +130,9 @@ out:
>
>         switch (undo) {
>         case 4:
> -               ok(sigaction(SIGSEGV, &sa_save, 0), 0);
> +               ok(sigaction(SIGSEGV, &sa_save, NULL), 0);
>         case 3:
> -               ok(sigaltstack(&ss_save, 0), 0);
> +               ok(sigaltstack(&ss_save, NULL), 0);
>         case 2:
>                 ok(munmap(m, max), 0);
>         case 1:
> diff --git a/ccan/altstack/altstack.h b/ccan/altstack/altstack.h
> index 4445a2a..09ffe0b 100644
> --- a/ccan/altstack/altstack.h
> +++ b/ccan/altstack/altstack.h
> @@ -52,7 +52,7 @@
>   *     static void *wrap(void *i)
>   *     {
>   *             dn((unsigned long) i);
> - *             return 0;
> + *             return NULL;
>   *     }
>   *
>   *     #define MiB (1024UL*1024UL)
> @@ -60,9 +60,9 @@
>   *     {
>   *             unsigned long n;
>   *             assert(argc == 2);
> - *             n = strtoul(argv[1], 0, 0);
> + *             n = strtoul(argv[1], NULL, 0);
>   *
> - *             if (altstack(32*MiB, wrap, (void *) n, 0) != 0)
> + *             if (altstack(32*MiB, wrap, (void *) n, NULL) != 0)
>   *                     altstack_perror();
>   *
>   *             printf("%d\n", depth);
> diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> index 61710fd..e109ccb 100644
> --- a/ccan/altstack/test/run.c
> +++ b/ccan/altstack/test/run.c
> @@ -87,43 +87,43 @@ int main(void)
>
>         plan_tests(50);
>
> -       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0), 0) ==
> -1, e(getrlimit_),
> +       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(getrlimit_),
>                 0,
>                 0);
>
> -       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0), 0) ==
> -1, e(setrlimit_),
> +       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(setrlimit_),
>                 getrlimit_,
>                 0);
>
> -       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0), 0) ==
> -1, e(mmap_),
> +       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(mmap_),
>                 getrlimit_|setrlimit_,
>                 setrlimit_);
>
> -       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0), 0) ==
> -1, e(sigaltstack_),
> +       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(sigaltstack_),
>                 getrlimit_|setrlimit_|mmap_,
>                 setrlimit_|munmap_);
>
> -       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0), 0) ==
> -1, e(sigaction_),
> +       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(sigaction_),
>                 getrlimit_|setrlimit_|mmap_|sigaltstack_,
>                 setrlimit_|munmap_|sigaltstack_);
>
> -       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), 0) ==
> 1, e(munmap_),
> +       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> ==  1, e(munmap_),
>                 getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
>                 setrlimit_|sigaltstack_|sigaction_);
>         if (fail = 0, munmap(m_, msz_) == -1)
>                 err(1, "munmap");
>
> -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000), 0)
> == -1, EOVERFLOW,
> +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
>                 getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
>                 setrlimit_|munmap_|sigaltstack_|sigaction_);
>
>         // be sure segv catch is repeatable (SA_NODEFER)
> -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000), 0)
> == -1, EOVERFLOW,
> +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
>                 getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
>                 setrlimit_|munmap_|sigaltstack_|sigaction_);
>
>         used = 1;
> -       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000), 0)
> == -1, EOVERFLOW,
> +       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
>                 getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
>                 setrlimit_|sigaltstack_|sigaction_);
>         if (fail = 0, munmap(m_, msz_) == -1)
> @@ -150,7 +150,7 @@ int main(void)
>         ok1(strcmp(buf, estr "\n") == 0);
>
>         used = 1;
> -       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000), 0)
> == -1, EOVERFLOW,
> +       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
>                 getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
>                 setrlimit_|munmap_|sigaltstack_|sigaction_);
>
> @@ -158,7 +158,7 @@ int main(void)
>         ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
>
>         used = 0;
> -       chkok(                  altstack(8*MiB, wrap, int2ptr(100000), 0)
> == 0, 0,
> +       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> NULL) == 0, 0,
>
> getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
>                 setrlimit_|munmap_|sigaltstack_|sigaction_);
>
> --
> 2.5.5
>
>

_______________________________________________
ccan mailing list
[email protected]
https://lists.ozlabs.org/listinfo/ccan