[ccan] [PATCH 4/5] altstack: Don't use 0 pointer literals
Dan Good
dan at dancancode.com
Sun Jun 12 11:27:12 AEST 2016
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 <david at gibson.dropbear.id.au>
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 <david at gibson.dropbear.id.au>
> ---
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160612/f76d9742/attachment.html>
More information about the ccan
mailing list