[PATCH 0/5] altstack: cleanups

[PATCH 0/5] altstack: cleanups

[David Gibson]

Dan,

Here are a bunch of assorted cleanups to the altstack module.  If they
seem reasonable to you, please apply.

David Gibson (5):
  altstack: Consolidate thread-local variables
  altstack: Restore alternate signal stack state
  altstack: Use ptrint instead of bare casts
  altstack: Don't use 0 pointer literals
  altstack: Don't log internal calls in test cases

 ccan/altstack/_info      |  9 ++++--
 ccan/altstack/altstack.c | 78 +++++++++++++++++++++++++-----------------------
 ccan/altstack/altstack.h |  6 ++--
 ccan/altstack/test/run.c | 76 ++++++++++++++--------------------------------
 4 files changed, 74 insertions(+), 95 deletions(-)

-- 
2.5.5

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 1/5] altstack: Consolidate thread-local variables

[David Gibson]

altstack uses a number of __thread variables to track internal state.  This
allows altstack to be thread-safe, although it's still not re-entrant.
This patch gathers all these variables into a single per-thread state
structure.  This makes it easy to see at a glance what the whole of the
required state is, and thereby easier to reason about correctness of
changes to the implementation.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/altstack.c | 67 +++++++++++++++++++++++++-----------------------
 ccan/altstack/test/run.c |  2 +-
 2 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
index 6351293..080cd50 100644
--- a/ccan/altstack/altstack.c
+++ b/ccan/altstack/altstack.c
@@ -11,43 +11,48 @@
 #include <unistd.h>
 #include <sys/mman.h>
 
-static __thread char ebuf[ALTSTACK_ERR_MAXLEN];
-static __thread unsigned elen;
-
-#define bang(x)							\
-	(elen += snprintf(ebuf + elen, sizeof(ebuf) - elen,	\
-		 "%s(altstack@%d) %s%s%s",			\
-		 elen  ? "; " : "", __LINE__, (x),		\
-		 errno ? ": " : "", errno ? strerror(errno) : ""))
+static __thread struct altstack_state {
+	char ebuf[ALTSTACK_ERR_MAXLEN];
+	unsigned elen;
+	jmp_buf jmp;
+	void *rsp_save_[2];
+	rlim_t max;
+	void *(*fn)(void *);
+	void *arg, *out;
+} state;
+
+#define bang(x)								\
+	(state.elen += snprintf(state.ebuf + state.elen,		\
+				sizeof(state.ebuf) - state.elen,	\
+				"%s(altstack@%d) %s%s%s",		\
+				state.elen  ? "; " : "", __LINE__, (x),	\
+				errno ? ": " : "",			\
+				errno ? strerror(errno) : ""))
 
 void altstack_perror(void)
 {
-	fprintf(stderr, "%s\n", ebuf);
+	fprintf(stderr, "%s\n", state.ebuf);
 }
 
 char *altstack_geterr(void)
 {
-	return ebuf;
+	return state.ebuf;
 }
 
-static __thread jmp_buf jmp;
-
 static void segvjmp(int signum)
 {
-	longjmp(jmp, 1);
+	longjmp(state.jmp, 1);
 }
 
-static __thread void *rsp_save_[2];
-static __thread rlim_t max_;
 
 rlim_t altstack_max(void) {
-	return max_;
+	return state.max;
 }
 
 static ptrdiff_t rsp_save(unsigned i) {
 	assert(i < 2);
-	asm volatile ("movq %%rsp, %0" : "=g" (rsp_save_[i]));
-	return (char *) rsp_save_[0] - (char *) rsp_save_[i];
+	asm volatile ("movq %%rsp, %0" : "=g" (state.rsp_save_[i]));
+	return (char *) state.rsp_save_[0] - (char *) state.rsp_save_[i];
 }
 
 void altstack_rsp_save(void) {
@@ -58,9 +63,6 @@ ptrdiff_t altstack_used(void) {
 	return rsp_save(1);
 }
 
-static __thread void *(*fn_)(void *);
-static __thread void *arg_, *out_;
-
 int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 {
 	long pgsz = sysconf(_SC_PAGESIZE);
@@ -73,11 +75,11 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 	assert(max > 0 && fn);
 	#define ok(x, y) ({ long __r = (long) (x); if (__r == -1) { bang(#x); if (y) goto out; } __r; })
 
-	fn_  = fn;
-	arg_ = arg;
-	out_ = 0;
-	max_ = max;
-	ebuf[elen = 0] = '\0';
+	state.fn  = fn;
+	state.arg = arg;
+	state.out = 0;
+	state.max = max;
+	state.ebuf[state.elen = 0] = '\0';
 	if (out) *out = 0;
 
 	// if the first page below the mapping is in use, we get max-pgsz usable bytes
@@ -85,13 +87,13 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 	max += pgsz;
 
 	ok(getrlimit(RLIMIT_STACK, &rl_save), 1);
-	ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max_, rl_save.rlim_max }), 1);
+	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);
 	undo++;
 
-	if (setjmp(jmp) == 0) {
+	if (setjmp(state.jmp) == 0) {
 		unsigned char sigstk[SIGSTKSZ];
 		stack_t ss = { .ss_sp = sigstk, .ss_size = sizeof(sigstk) };
 		struct sigaction sa = { .sa_handler = segvjmp, .sa_flags = SA_NODEFER|SA_RESETHAND|SA_ONSTACK };
@@ -108,12 +110,13 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 			"mov %1, %%rsp\n\t"
 			"sub $8, %%rsp\n\t"
 			"push %%r10"
-			: "=r" (rsp_save_[0]) : "0" (m + max) : "r10", "memory");
-		out_ = fn_(arg_);
+			: "=r" (state.rsp_save_[0])
+			: "0" (m + max) : "r10", "memory");
+		state.out = state.fn(state.arg);
 		asm volatile ("pop %%rsp"
 			      : : : "memory");
 		ret = 0;
-		if (out) *out = out_;
+		if (out) *out = state.out;
 	}
 	else {
 		errno = 0;
@@ -137,5 +140,5 @@ out:
 
 	if (errno_save)
 		errno = errno_save;
-	return !ret && elen ? 1 : ret;
+	return !ret && state.elen ? 1 : ret;
 }
diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
index 12cc460..23dd2e9 100644
--- a/ccan/altstack/test/run.c
+++ b/ccan/altstack/test/run.c
@@ -24,7 +24,7 @@ char *m_;
 rlim_t msz_;
 #define e(x) (900+(x))
 #define seterr(x) (errno = e(x))
-#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno || out_ ? (x) : 0))
+#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno || state.out ? (x) : 0))
 #define getrlimit(...)		(fail&getrlimit_	? (seterr(getrlimit_),		-1) : (setcall(getrlimit_),	getrlimit(__VA_ARGS__)))
 #define mmap(...)		(fail&mmap_		? (seterr(mmap_),	(void *)-1) : (setcall(mmap_),		mmap(__VA_ARGS__)))
 #define munmap(a, b)		(fail&munmap_		? (seterr(munmap_),		-1) : (setcall(munmap_),	munmap(m_=(a), msz_=(b))))
-- 
2.5.5

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 2/5] altstack: Restore alternate signal stack state

[David Gibson]

altstack relies on catching a SIGSEGV caused when overrunning the stack.
This means that the SEGV handler itself can't use the already overflowed
stack, and so we use sigaltstack() to assign the signal handler a different
stack.  On completion, altstack() clears the alternate signal stack.

However, it's possible that the calling program could be using
sigaltstack() for its own reasons, so it's more correct to restore the
sigaltstack() state to that from the beginning of the altstack() call.
This patch implements this behaviour.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/altstack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
index 080cd50..6dfb9fa 100644
--- a/ccan/altstack/altstack.c
+++ b/ccan/altstack/altstack.c
@@ -71,6 +71,7 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 	struct rlimit rl_save;
 	struct sigaction sa_save;
 	int errno_save;
+	stack_t ss_save;
 
 	assert(max > 0 && fn);
 	#define ok(x, y) ({ long __r = (long) (x); if (__r == -1) { bang(#x); if (y) goto out; } __r; })
@@ -98,7 +99,7 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 		stack_t ss = { .ss_sp = sigstk, .ss_size = sizeof(sigstk) };
 		struct sigaction sa = { .sa_handler = segvjmp, .sa_flags = SA_NODEFER|SA_RESETHAND|SA_ONSTACK };
 
-		ok(sigaltstack(&ss, 0), 1);
+		ok(sigaltstack(&ss, &ss_save), 1);
 		undo++;
 
 		sigemptyset(&sa.sa_mask);
@@ -131,7 +132,7 @@ out:
 	case 4:
 		ok(sigaction(SIGSEGV, &sa_save, 0), 0);
 	case 3:
-		ok(sigaltstack(&(stack_t) { .ss_flags = SS_DISABLE }, 0), 0);
+		ok(sigaltstack(&ss_save, 0), 0);
 	case 2:
 		ok(munmap(m, max), 0);
 	case 1:
-- 
2.5.5

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 3/5] altstack: Use ptrint instead of bare casts

[David Gibson]

Functions invoked with altstack take a void * parameter.  However, the
test program wants to pass an integer, and so uses the trick of casting
the integer values to (void *) and back again.

The ptrint() module handles exactly this case in a more portable and
(somewhat) typesafe way, so use that instead.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/_info      |  5 +++++
 ccan/altstack/test/run.c | 25 +++++++++++++------------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/ccan/altstack/_info b/ccan/altstack/_info
index 2713bd0..7accf86 100644
--- a/ccan/altstack/_info
+++ b/ccan/altstack/_info
@@ -122,6 +122,11 @@ int main(int argc, char *argv[])
 	if (strcmp(argv[1], "depends") == 0)
 		return 0;
 
+	if (strcmp(argv[1], "testdepends") == 0) {
+		printf("ccan/ptrint\n");
+		return 0;
+	}
+
 	if (strcmp(argv[1], "ported") == 0) {
 #ifdef __x86_64__
 		printf("\n");
diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
index 23dd2e9..61710fd 100644
--- a/ccan/altstack/test/run.c
+++ b/ccan/altstack/test/run.c
@@ -8,6 +8,7 @@
 #include <unistd.h>
 #include <sys/mman.h>
 #include <ccan/tap/tap.h>
+#include <ccan/ptrint/ptrint.h>
 #include <ccan/altstack/altstack.h>
 #include <stdio.h>
 
@@ -53,7 +54,7 @@ static void __attribute__((optimize("O0"))) dn(unsigned long i)
 }
 static void *wrap(void *i)
 {
-	dn((unsigned long) i);
+	dn(ptr2int(i));
 	return wrap;
 }
 
@@ -86,43 +87,43 @@ int main(void)
 
 	plan_tests(50);
 
-	chkfail(getrlimit_,	altstack(8*MiB, wrap, 0, 0) == -1, e(getrlimit_),
+	chkfail(getrlimit_,	altstack(8*MiB, wrap, int2ptr(0), 0) == -1, e(getrlimit_),
 		0,
 		0);
 
-	chkfail(setrlimit_,	altstack(8*MiB, wrap, 0, 0) == -1, e(setrlimit_),
+	chkfail(setrlimit_,	altstack(8*MiB, wrap, int2ptr(0), 0) == -1, e(setrlimit_),
 		getrlimit_,
 		0);
 
-	chkfail(mmap_,		altstack(8*MiB, wrap, 0, 0) == -1, e(mmap_),
+	chkfail(mmap_,		altstack(8*MiB, wrap, int2ptr(0), 0) == -1, e(mmap_),
 		getrlimit_|setrlimit_,
 		setrlimit_);
 
-	chkfail(sigaltstack_,	altstack(8*MiB, wrap, 0, 0) == -1, e(sigaltstack_),
+	chkfail(sigaltstack_,	altstack(8*MiB, wrap, int2ptr(0), 0) == -1, e(sigaltstack_),
 		getrlimit_|setrlimit_|mmap_,
 		setrlimit_|munmap_);
 
-	chkfail(sigaction_,	altstack(8*MiB, wrap, 0, 0) == -1, e(sigaction_),
+	chkfail(sigaction_,	altstack(8*MiB, wrap, int2ptr(0), 0) == -1, e(sigaction_),
 		getrlimit_|setrlimit_|mmap_|sigaltstack_,
 		setrlimit_|munmap_|sigaltstack_);
 
-	chkfail(munmap_,	altstack(8*MiB, wrap, 0, 0) ==  1, e(munmap_),
+	chkfail(munmap_,	altstack(8*MiB, wrap, int2ptr(0), 0) ==  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, (void *) 1000000, 0) == -1, EOVERFLOW,
+	chkok(			altstack(1*MiB, wrap, int2ptr(1000000), 0) == -1, EOVERFLOW,
 		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
 		setrlimit_|munmap_|sigaltstack_|sigaction_);
 
 	// be sure segv catch is repeatable (SA_NODEFER)
-	chkok(			altstack(1*MiB, wrap, (void *) 1000000, 0) == -1, EOVERFLOW,
+	chkok(			altstack(1*MiB, wrap, int2ptr(1000000), 0) == -1, EOVERFLOW,
 		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
 		setrlimit_|munmap_|sigaltstack_|sigaction_);
 
 	used = 1;
-	chkfail(munmap_,	altstack(1*MiB, wrap, (void *) 1000000, 0) == -1, EOVERFLOW,
+	chkfail(munmap_,	altstack(1*MiB, wrap, int2ptr(1000000), 0) == -1, EOVERFLOW,
 		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
 		setrlimit_|sigaltstack_|sigaction_);
 	if (fail = 0, munmap(m_, msz_) == -1)
@@ -149,7 +150,7 @@ int main(void)
 	ok1(strcmp(buf, estr "\n") == 0);
 
 	used = 1;
-	chkok(			altstack(8*MiB, wrap, (void *) 1000000, 0) == -1, EOVERFLOW,
+	chkok(			altstack(8*MiB, wrap, int2ptr(1000000), 0) == -1, EOVERFLOW,
 		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
 		setrlimit_|munmap_|sigaltstack_|sigaction_);
 
@@ -157,7 +158,7 @@ int main(void)
 	ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
 
 	used = 0;
-	chkok(			altstack(8*MiB, wrap, (void *) 100000, 0) == 0, 0,
+	chkok(			altstack(8*MiB, wrap, int2ptr(100000), 0) == 0, 0,
 		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
 		setrlimit_|munmap_|sigaltstack_|sigaction_);
 
-- 
2.5.5

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

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

[David Gibson]

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

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

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

[Dan Good]

[-- Attachment #1.1: Type: text/plain, Size: 8504 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 10427 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

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

[Rusty Russell]

Dan Good <dan@dancancode.com> writes:
> 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).

(I don't really care: your code, your style).  But...

He is so horrifically wrong, it's amazing.

Section 7.17 is loosened from ANSI C which said NULL was 0, presumably
to *allow* compilers to avoid the varargs issue.  A compiler *could* do
insane shit to make that problem even worse, but you would never use
such a compiler.  There are other legal-but-insane things a compiler can
do, too, and the answer is the same; real code won't work, nobody else
cares.

OTOH, using 0 in place of NULL makes for much more potential type order
confusion.  Not to mention confusing every damn C programmer on the
planet.

Note, I also assume NULL is all zero-bits, that size_t can hold a
ptrdiff_t, and that a pointer to a function which takes a type *
argument can be cast and called as a function which takes a void *.

If someone ports CCAN to a platform where those are not true, I'd be
fascinated...

Cheers,
Rusty.
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

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

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 2170 bytes --]

On Tue, Jun 14, 2016 at 01:45:10PM +0930, Paul 'Rusty' Russell wrote:
> Dan Good <dan@dancancode.com> writes:
> > 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).
> 
> (I don't really care: your code, your style).  But...
> 
> He is so horrifically wrong, it's amazing.
> 
> Section 7.17 is loosened from ANSI C which said NULL was 0, presumably
> to *allow* compilers to avoid the varargs issue.  A compiler *could* do
> insane shit to make that problem even worse, but you would never use
> such a compiler.  There are other legal-but-insane things a compiler can
> do, too, and the answer is the same; real code won't work, nobody else
> cares.
> 
> OTOH, using 0 in place of NULL makes for much more potential type order
> confusion.  Not to mention confusing every damn C programmer on the
> planet.

Yeah.  I can kind of see the hint of a good idea in those articles,
but on balance they're really not convincing.  Basically, as Rusty
says, matching the conventions of the huge bulk of existing C code
outweighs the value of working with a compiler/library that has gone
out of its way to implement this stupidly.

> Note, I also assume NULL is all zero-bits,

I try to avoid that one, although I can't be sure I always have - I'm
not sure, but I think one of the s390 variants might break this.

> that size_t can hold a
> ptrdiff_t,

Dunno if I've assumed that much.

> and that a pointer to a function which takes a type *
> argument can be cast and called as a function which takes a void *.

Yeah, I've used that.

> If someone ports CCAN to a platform where those are not true, I'd be
> fascinated...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

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

[Dan Good]

[-- Attachment #1.1: Type: text/plain, Size: 2433 bytes --]

Thank you, both.  I'll return to the fold and use NULL, as seems right and
proper.

On Thu, Jun 16, 2016 at 7:12 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> On Tue, Jun 14, 2016 at 01:45:10PM +0930, Paul 'Rusty' Russell wrote:
> > Dan Good <dan@dancancode.com> writes:
> > > 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).
> >
> > (I don't really care: your code, your style).  But...
> >
> > He is so horrifically wrong, it's amazing.
> >
> > Section 7.17 is loosened from ANSI C which said NULL was 0, presumably
> > to *allow* compilers to avoid the varargs issue.  A compiler *could* do
> > insane shit to make that problem even worse, but you would never use
> > such a compiler.  There are other legal-but-insane things a compiler can
> > do, too, and the answer is the same; real code won't work, nobody else
> > cares.
> >
> > OTOH, using 0 in place of NULL makes for much more potential type order
> > confusion.  Not to mention confusing every damn C programmer on the
> > planet.
>
> Yeah.  I can kind of see the hint of a good idea in those articles,
> but on balance they're really not convincing.  Basically, as Rusty
> says, matching the conventions of the huge bulk of existing C code
> outweighs the value of working with a compiler/library that has gone
> out of its way to implement this stupidly.
>
> > Note, I also assume NULL is all zero-bits,
>
> I try to avoid that one, although I can't be sure I always have - I'm
> not sure, but I think one of the s390 variants might break this.
>
> > that size_t can hold a
> > ptrdiff_t,
>
> Dunno if I've assumed that much.
>
> > and that a pointer to a function which takes a type *
> > argument can be cast and called as a function which takes a void *.
>
> Yeah, I've used that.
>
> > If someone ports CCAN to a platform where those are not true, I'd be
> > fascinated...
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_
> _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

[-- Attachment #1.2: Type: text/html, Size: 3272 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[PATCH 5/5] altstack: Don't log internal calls in test cases

[David Gibson]

altstack/test/run.c uses some hairy macros to intercept the standard
library functions that altstack uses.  This has two purposes: 1) to
conditionally cause those functions to fail, and thereby test altstack's
error paths, and 2) log which of the library functions was called in each
testcase.

The second function isn't actually useful - for the purposes of testing the
module, we want to check the actual behaviour, not what calls it made in
what order to accomplish it.  Explicitly checking the calls makes it much
harder to change altstack's implementation without breaking the tests.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 ccan/altstack/test/run.c | 73 ++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 52 deletions(-)

diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
index e109ccb..091d1f5 100644
--- a/ccan/altstack/test/run.c
+++ b/ccan/altstack/test/run.c
@@ -20,18 +20,17 @@ enum {
 	sigaction_	= 1<<4,
 	munmap_		= 1<<5,
 };
-int fail, call1, call2;
+int fail;
 char *m_;
 rlim_t msz_;
 #define e(x) (900+(x))
 #define seterr(x) (errno = e(x))
-#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno || state.out ? (x) : 0))
-#define getrlimit(...)		(fail&getrlimit_	? (seterr(getrlimit_),		-1) : (setcall(getrlimit_),	getrlimit(__VA_ARGS__)))
-#define mmap(...)		(fail&mmap_		? (seterr(mmap_),	(void *)-1) : (setcall(mmap_),		mmap(__VA_ARGS__)))
-#define munmap(a, b)		(fail&munmap_		? (seterr(munmap_),		-1) : (setcall(munmap_),	munmap(m_=(a), msz_=(b))))
-#define setrlimit(...)		(fail&setrlimit_	? (seterr(setrlimit_),		-1) : (setcall(setrlimit_),	setrlimit(__VA_ARGS__)))
-#define sigaltstack(...)	(fail&sigaltstack_	? (seterr(sigaltstack_),	-1) : (setcall(sigaltstack_),	sigaltstack(__VA_ARGS__)))
-#define sigaction(...)		(fail&sigaction_	? (seterr(sigaction_),		-1) : (setcall(sigaction_),	sigaction(__VA_ARGS__)))
+#define getrlimit(...)		(fail&getrlimit_	? (seterr(getrlimit_),		-1) : getrlimit(__VA_ARGS__))
+#define mmap(...)		(fail&mmap_		? (seterr(mmap_),	(void *)-1) : mmap(__VA_ARGS__))
+#define munmap(a, b)		(fail&munmap_		? (seterr(munmap_),		-1) : munmap(m_=(a), msz_=(b)))
+#define setrlimit(...)		(fail&setrlimit_	? (seterr(setrlimit_),		-1) : setrlimit(__VA_ARGS__))
+#define sigaltstack(...)	(fail&sigaltstack_	? (seterr(sigaltstack_),	-1) : sigaltstack(__VA_ARGS__))
+#define sigaction(...)		(fail&sigaction_	? (seterr(sigaction_),		-1) : sigaction(__VA_ARGS__))
 
 #define KiB (1024UL)
 #define MiB (KiB*KiB)
@@ -58,74 +57,48 @@ static void *wrap(void *i)
 	return wrap;
 }
 
-#define chkfail(x, y, z, c1, c2)					\
+#define chkfail(x, y, z)						\
 	do {								\
-		call1 = 0;						\
-		call2 = 0;						\
 		errno = 0;						\
 		ok1((fail = x) && (y));					\
 		ok1(errno == (z));					\
-		ok1(call1 == (c1));					\
-		ok1(call2 == (c2));					\
 	} while (0);
 
-#define chkok(y, z, c1, c2)						\
+#define chkok(y, z)							\
 	do {								\
-		call1 = 0;						\
-		call2 = 0;						\
 		errno = 0;						\
 		fail = 0;						\
 		ok1((y));						\
 		ok1(errno == (z));					\
-		ok1(call1 == (c1));					\
-		ok1(call2 == (c2));					\
 	} while (0)
 
 int main(void)
 {
 	long pgsz = sysconf(_SC_PAGESIZE);
 
-	plan_tests(50);
+	plan_tests(28);
 
-	chkfail(getrlimit_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(getrlimit_),
-		0,
-		0);
+	chkfail(getrlimit_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(getrlimit_));
 
-	chkfail(setrlimit_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(setrlimit_),
-		getrlimit_,
-		0);
+	chkfail(setrlimit_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(setrlimit_));
 
-	chkfail(mmap_,		altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(mmap_),
-		getrlimit_|setrlimit_,
-		setrlimit_);
+	chkfail(mmap_,		altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(mmap_));
 
-	chkfail(sigaltstack_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(sigaltstack_),
-		getrlimit_|setrlimit_|mmap_,
-		setrlimit_|munmap_);
+	chkfail(sigaltstack_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(sigaltstack_));
 
-	chkfail(sigaction_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(sigaction_),
-		getrlimit_|setrlimit_|mmap_|sigaltstack_,
-		setrlimit_|munmap_|sigaltstack_);
+	chkfail(sigaction_,	altstack(8*MiB, wrap, int2ptr(0), NULL) == -1, e(sigaction_));
 
-	chkfail(munmap_,	altstack(8*MiB, wrap, int2ptr(0), NULL) ==  1, e(munmap_),
-		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
-		setrlimit_|sigaltstack_|sigaction_);
+	chkfail(munmap_,	altstack(8*MiB, wrap, int2ptr(0), NULL) ==  1, e(munmap_));
 	if (fail = 0, munmap(m_, msz_) == -1)
 		err(1, "munmap");
 
-	chkok(			altstack(1*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW,
-		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
-		setrlimit_|munmap_|sigaltstack_|sigaction_);
+	chkok(			altstack(1*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW);
 
 	// be sure segv catch is repeatable (SA_NODEFER)
-	chkok(			altstack(1*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW,
-		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
-		setrlimit_|munmap_|sigaltstack_|sigaction_);
+	chkok(			altstack(1*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW);
 
 	used = 1;
-	chkfail(munmap_,	altstack(1*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW,
-		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
-		setrlimit_|sigaltstack_|sigaction_);
+	chkfail(munmap_,	altstack(1*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW);
 	if (fail = 0, munmap(m_, msz_) == -1)
 		err(1, "munmap");
 
@@ -150,17 +123,13 @@ int main(void)
 	ok1(strcmp(buf, estr "\n") == 0);
 
 	used = 1;
-	chkok(			altstack(8*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW,
-		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
-		setrlimit_|munmap_|sigaltstack_|sigaction_);
+	chkok(			altstack(8*MiB, wrap, int2ptr(1000000), NULL) == -1, EOVERFLOW);
 
 	diag("used: %lu", used);
 	ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
 
 	used = 0;
-	chkok(			altstack(8*MiB, wrap, int2ptr(100000), NULL) == 0, 0,
-		getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
-		setrlimit_|munmap_|sigaltstack_|sigaction_);
+	chkok(			altstack(8*MiB, wrap, int2ptr(100000), NULL) == 0, 0);
 
 	used = 1;
 	altstack_rsp_save();
-- 
2.5.5

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 5/5] altstack: Don't log internal calls in test cases

[Dan Good]

[-- Attachment #1.1: Type: text/plain, Size: 9382 bytes --]

Hairy macros?  From the author of the cppmagic module, I shall take that as
a compliment.

The purpose of the setcall macro and related checks is ensure the
correctness of the error path, i.e. if setrlimit ran before a failure, it
must run again to undo the first; if mmap ran before a failure, munmap must
run after.  I find it very reassuring to know these tests exist and pass.
Do you see no value in that?

I don't really see a need to optimize for changing altstack's
implementation.  It's less than a hundred lines of code, and only ten tests
using setcall.  Can you tell me why you think it's important?

On Fri, Jun 3, 2016 at 4:40 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> altstack/test/run.c uses some hairy macros to intercept the standard
> library functions that altstack uses.  This has two purposes: 1) to
> conditionally cause those functions to fail, and thereby test altstack's
> error paths, and 2) log which of the library functions was called in each
> testcase.
>
> The second function isn't actually useful - for the purposes of testing the
> module, we want to check the actual behaviour, not what calls it made in
> what order to accomplish it.  Explicitly checking the calls makes it much
> harder to change altstack's implementation without breaking the tests.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  ccan/altstack/test/run.c | 73
> ++++++++++++++----------------------------------
>  1 file changed, 21 insertions(+), 52 deletions(-)
>
> diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> index e109ccb..091d1f5 100644
> --- a/ccan/altstack/test/run.c
> +++ b/ccan/altstack/test/run.c
> @@ -20,18 +20,17 @@ enum {
>         sigaction_      = 1<<4,
>         munmap_         = 1<<5,
>  };
> -int fail, call1, call2;
> +int fail;
>  char *m_;
>  rlim_t msz_;
>  #define e(x) (900+(x))
>  #define seterr(x) (errno = e(x))
> -#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno ||
> state.out ? (x) : 0))
> -#define getrlimit(...)         (fail&getrlimit_        ?
> (seterr(getrlimit_),          -1) : (setcall(getrlimit_),
>  getrlimit(__VA_ARGS__)))
> -#define mmap(...)              (fail&mmap_             ? (seterr(mmap_),
>      (void *)-1) : (setcall(mmap_),          mmap(__VA_ARGS__)))
> -#define munmap(a, b)           (fail&munmap_           ?
> (seterr(munmap_),             -1) : (setcall(munmap_),
> munmap(m_=(a), msz_=(b))))
> -#define setrlimit(...)         (fail&setrlimit_        ?
> (seterr(setrlimit_),          -1) : (setcall(setrlimit_),
>  setrlimit(__VA_ARGS__)))
> -#define sigaltstack(...)       (fail&sigaltstack_      ?
> (seterr(sigaltstack_),        -1) : (setcall(sigaltstack_),
>  sigaltstack(__VA_ARGS__)))
> -#define sigaction(...)         (fail&sigaction_        ?
> (seterr(sigaction_),          -1) : (setcall(sigaction_),
>  sigaction(__VA_ARGS__)))
> +#define getrlimit(...)         (fail&getrlimit_        ?
> (seterr(getrlimit_),          -1) : getrlimit(__VA_ARGS__))
> +#define mmap(...)              (fail&mmap_             ? (seterr(mmap_),
>      (void *)-1) : mmap(__VA_ARGS__))
> +#define munmap(a, b)           (fail&munmap_           ?
> (seterr(munmap_),             -1) : munmap(m_=(a), msz_=(b)))
> +#define setrlimit(...)         (fail&setrlimit_        ?
> (seterr(setrlimit_),          -1) : setrlimit(__VA_ARGS__))
> +#define sigaltstack(...)       (fail&sigaltstack_      ?
> (seterr(sigaltstack_),        -1) : sigaltstack(__VA_ARGS__))
> +#define sigaction(...)         (fail&sigaction_        ?
> (seterr(sigaction_),          -1) : sigaction(__VA_ARGS__))
>
>  #define KiB (1024UL)
>  #define MiB (KiB*KiB)
> @@ -58,74 +57,48 @@ static void *wrap(void *i)
>         return wrap;
>  }
>
> -#define chkfail(x, y, z, c1, c2)                                       \
> +#define chkfail(x, y, z)                                               \
>         do {                                                            \
> -               call1 = 0;                                              \
> -               call2 = 0;                                              \
>                 errno = 0;                                              \
>                 ok1((fail = x) && (y));                                 \
>                 ok1(errno == (z));                                      \
> -               ok1(call1 == (c1));                                     \
> -               ok1(call2 == (c2));                                     \
>         } while (0);
>
> -#define chkok(y, z, c1, c2)                                            \
> +#define chkok(y, z)                                                    \
>         do {                                                            \
> -               call1 = 0;                                              \
> -               call2 = 0;                                              \
>                 errno = 0;                                              \
>                 fail = 0;                                               \
>                 ok1((y));                                               \
>                 ok1(errno == (z));                                      \
> -               ok1(call1 == (c1));                                     \
> -               ok1(call2 == (c2));                                     \
>         } while (0)
>
>  int main(void)
>  {
>         long pgsz = sysconf(_SC_PAGESIZE);
>
> -       plan_tests(50);
> +       plan_tests(28);
>
> -       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(getrlimit_),
> -               0,
> -               0);
> +       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(getrlimit_));
>
> -       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(setrlimit_),
> -               getrlimit_,
> -               0);
> +       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(setrlimit_));
>
> -       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(mmap_),
> -               getrlimit_|setrlimit_,
> -               setrlimit_);
> +       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(mmap_));
>
> -       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(sigaltstack_),
> -               getrlimit_|setrlimit_|mmap_,
> -               setrlimit_|munmap_);
> +       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(sigaltstack_));
>
> -       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(sigaction_),
> -               getrlimit_|setrlimit_|mmap_|sigaltstack_,
> -               setrlimit_|munmap_|sigaltstack_);
> +       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> -1, e(sigaction_));
>
> -       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> ==  1, e(munmap_),
> -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> -               setrlimit_|sigaltstack_|sigaction_);
> +       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> ==  1, e(munmap_));
>         if (fail = 0, munmap(m_, msz_) == -1)
>                 err(1, "munmap");
>
> -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
> -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW);
>
>         // be sure segv catch is repeatable (SA_NODEFER)
> -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
> -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW);
>
>         used = 1;
> -       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
> -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> -               setrlimit_|sigaltstack_|sigaction_);
> +       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW);
>         if (fail = 0, munmap(m_, msz_) == -1)
>                 err(1, "munmap");
>
> @@ -150,17 +123,13 @@ int main(void)
>         ok1(strcmp(buf, estr "\n") == 0);
>
>         used = 1;
> -       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW,
> -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> +       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> NULL) == -1, EOVERFLOW);
>
>         diag("used: %lu", used);
>         ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
>
>         used = 0;
> -       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> NULL) == 0, 0,
> -
>  getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
> -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> +       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> NULL) == 0, 0);
>
>         used = 1;
>         altstack_rsp_save();
> --
> 2.5.5
>
>

[-- Attachment #1.2: Type: text/html, Size: 11651 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 5/5] altstack: Don't log internal calls in test cases

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 11894 bytes --]

On Sun, Jun 12, 2016 at 02:10:18AM +0000, Dan Good wrote:
> Hairy macros?  From the author of the cppmagic module, I shall take that as
> a compliment.

Touché.

That said, cppmagic is using hairy magic to do some things that are,
as far as I know, impossible by any other method.  I don't think
there's a similar justification here.

> The purpose of the setcall macro and related checks is ensure the
> correctness of the error path, i.e. if setrlimit ran before a failure, it
> must run again to undo the first; if mmap ran before a failure, munmap must
> run after.  I find it very reassuring to know these tests exist and pass.
> Do you see no value in that?

So, there's certainly value in checking the error paths, but doing it
by checking what calls the implementation makes is not a great way of
doing it.  It's pretty subject to both false positives and false
negatives.

What I'd suggest instead is actually checking the behaviour in
question.  For example, if you want to check that the rlimit is
restored properly I'd suggest:
        1. Call getrlimit()
	2. Call altstack() in a way rigged to fail
	3. Call getrlimit() again
	4. Compare results from (1) and (3)

> I don't really see a need to optimize for changing altstack's
> implementation.  It's less than a hundred lines of code, and only ten tests
> using setcall.  Can you tell me why you think it's important?

So, the checking of specific calls makes the tests very dependent on
altstack's implementation, and the framework of macros used to do it
makes it difficult to change one test without changing them all.

Together, that makes it almost impossible to change anything
significant about the altstack implementation without having to
significantly rewrite the tests.  And if the tests are significantly
rewritten, it's hard to be confident that they still check the things
they used to.

Which undermines the whole value of a testsuite in allowing you to
modify the implementation while being reasonably confident you haven't
changed the desired behaviour.

This is not theoretical; I have a couple of changes in mind for which
the primary obstacle is adjusting the testsuite to match (switching to
ccan/coroutine to avoid the x86_64 specific code, and using mprotect()
instead of MAP_GROWSDOWN+setrlimit()).

> On Fri, Jun 3, 2016 at 4:40 AM David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > altstack/test/run.c uses some hairy macros to intercept the standard
> > library functions that altstack uses.  This has two purposes: 1) to
> > conditionally cause those functions to fail, and thereby test altstack's
> > error paths, and 2) log which of the library functions was called in each
> > testcase.
> >
> > The second function isn't actually useful - for the purposes of testing the
> > module, we want to check the actual behaviour, not what calls it made in
> > what order to accomplish it.  Explicitly checking the calls makes it much
> > harder to change altstack's implementation without breaking the tests.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  ccan/altstack/test/run.c | 73
> > ++++++++++++++----------------------------------
> >  1 file changed, 21 insertions(+), 52 deletions(-)
> >
> > diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> > index e109ccb..091d1f5 100644
> > --- a/ccan/altstack/test/run.c
> > +++ b/ccan/altstack/test/run.c
> > @@ -20,18 +20,17 @@ enum {
> >         sigaction_      = 1<<4,
> >         munmap_         = 1<<5,
> >  };
> > -int fail, call1, call2;
> > +int fail;
> >  char *m_;
> >  rlim_t msz_;
> >  #define e(x) (900+(x))
> >  #define seterr(x) (errno = e(x))
> > -#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno ||
> > state.out ? (x) : 0))
> > -#define getrlimit(...)         (fail&getrlimit_        ?
> > (seterr(getrlimit_),          -1) : (setcall(getrlimit_),
> >  getrlimit(__VA_ARGS__)))
> > -#define mmap(...)              (fail&mmap_             ? (seterr(mmap_),
> >      (void *)-1) : (setcall(mmap_),          mmap(__VA_ARGS__)))
> > -#define munmap(a, b)           (fail&munmap_           ?
> > (seterr(munmap_),             -1) : (setcall(munmap_),
> > munmap(m_=(a), msz_=(b))))
> > -#define setrlimit(...)         (fail&setrlimit_        ?
> > (seterr(setrlimit_),          -1) : (setcall(setrlimit_),
> >  setrlimit(__VA_ARGS__)))
> > -#define sigaltstack(...)       (fail&sigaltstack_      ?
> > (seterr(sigaltstack_),        -1) : (setcall(sigaltstack_),
> >  sigaltstack(__VA_ARGS__)))
> > -#define sigaction(...)         (fail&sigaction_        ?
> > (seterr(sigaction_),          -1) : (setcall(sigaction_),
> >  sigaction(__VA_ARGS__)))
> > +#define getrlimit(...)         (fail&getrlimit_        ?
> > (seterr(getrlimit_),          -1) : getrlimit(__VA_ARGS__))
> > +#define mmap(...)              (fail&mmap_             ? (seterr(mmap_),
> >      (void *)-1) : mmap(__VA_ARGS__))
> > +#define munmap(a, b)           (fail&munmap_           ?
> > (seterr(munmap_),             -1) : munmap(m_=(a), msz_=(b)))
> > +#define setrlimit(...)         (fail&setrlimit_        ?
> > (seterr(setrlimit_),          -1) : setrlimit(__VA_ARGS__))
> > +#define sigaltstack(...)       (fail&sigaltstack_      ?
> > (seterr(sigaltstack_),        -1) : sigaltstack(__VA_ARGS__))
> > +#define sigaction(...)         (fail&sigaction_        ?
> > (seterr(sigaction_),          -1) : sigaction(__VA_ARGS__))
> >
> >  #define KiB (1024UL)
> >  #define MiB (KiB*KiB)
> > @@ -58,74 +57,48 @@ static void *wrap(void *i)
> >         return wrap;
> >  }
> >
> > -#define chkfail(x, y, z, c1, c2)                                       \
> > +#define chkfail(x, y, z)                                               \
> >         do {                                                            \
> > -               call1 = 0;                                              \
> > -               call2 = 0;                                              \
> >                 errno = 0;                                              \
> >                 ok1((fail = x) && (y));                                 \
> >                 ok1(errno == (z));                                      \
> > -               ok1(call1 == (c1));                                     \
> > -               ok1(call2 == (c2));                                     \
> >         } while (0);
> >
> > -#define chkok(y, z, c1, c2)                                            \
> > +#define chkok(y, z)                                                    \
> >         do {                                                            \
> > -               call1 = 0;                                              \
> > -               call2 = 0;                                              \
> >                 errno = 0;                                              \
> >                 fail = 0;                                               \
> >                 ok1((y));                                               \
> >                 ok1(errno == (z));                                      \
> > -               ok1(call1 == (c1));                                     \
> > -               ok1(call2 == (c2));                                     \
> >         } while (0)
> >
> >  int main(void)
> >  {
> >         long pgsz = sysconf(_SC_PAGESIZE);
> >
> > -       plan_tests(50);
> > +       plan_tests(28);
> >
> > -       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(getrlimit_),
> > -               0,
> > -               0);
> > +       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(getrlimit_));
> >
> > -       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(setrlimit_),
> > -               getrlimit_,
> > -               0);
> > +       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(setrlimit_));
> >
> > -       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(mmap_),
> > -               getrlimit_|setrlimit_,
> > -               setrlimit_);
> > +       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(mmap_));
> >
> > -       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(sigaltstack_),
> > -               getrlimit_|setrlimit_|mmap_,
> > -               setrlimit_|munmap_);
> > +       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(sigaltstack_));
> >
> > -       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(sigaction_),
> > -               getrlimit_|setrlimit_|mmap_|sigaltstack_,
> > -               setrlimit_|munmap_|sigaltstack_);
> > +       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0), NULL) ==
> > -1, e(sigaction_));
> >
> > -       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> > ==  1, e(munmap_),
> > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > -               setrlimit_|sigaltstack_|sigaction_);
> > +       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> > ==  1, e(munmap_));
> >         if (fail = 0, munmap(m_, msz_) == -1)
> >                 err(1, "munmap");
> >
> > -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW,
> > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW);
> >
> >         // be sure segv catch is repeatable (SA_NODEFER)
> > -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW,
> > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW);
> >
> >         used = 1;
> > -       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW,
> > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > -               setrlimit_|sigaltstack_|sigaction_);
> > +       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW);
> >         if (fail = 0, munmap(m_, msz_) == -1)
> >                 err(1, "munmap");
> >
> > @@ -150,17 +123,13 @@ int main(void)
> >         ok1(strcmp(buf, estr "\n") == 0);
> >
> >         used = 1;
> > -       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW,
> > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > +       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> > NULL) == -1, EOVERFLOW);
> >
> >         diag("used: %lu", used);
> >         ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
> >
> >         used = 0;
> > -       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> > NULL) == 0, 0,
> > -
> >  getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
> > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > +       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> > NULL) == 0, 0);
> >
> >         used = 1;
> >         altstack_rsp_save();
> >
> >

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 5/5] altstack: Don't log internal calls in test cases

[Dan Good]

[-- Attachment #1.1: Type: text/plain, Size: 11925 bytes --]

Very well, I've applied your patches as provided (except I dropped the
trailing underscore from state.rsp_save).

On Thu, Jun 16, 2016 at 7:12 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> On Sun, Jun 12, 2016 at 02:10:18AM +0000, Dan Good wrote:
> > Hairy macros?  From the author of the cppmagic module, I shall take that
> as
> > a compliment.
>
> Touché.
>
> That said, cppmagic is using hairy magic to do some things that are,
> as far as I know, impossible by any other method.  I don't think
> there's a similar justification here.
>
> > The purpose of the setcall macro and related checks is ensure the
> > correctness of the error path, i.e. if setrlimit ran before a failure, it
> > must run again to undo the first; if mmap ran before a failure, munmap
> must
> > run after.  I find it very reassuring to know these tests exist and pass.
> > Do you see no value in that?
>
> So, there's certainly value in checking the error paths, but doing it
> by checking what calls the implementation makes is not a great way of
> doing it.  It's pretty subject to both false positives and false
> negatives.
>
> What I'd suggest instead is actually checking the behaviour in
> question.  For example, if you want to check that the rlimit is
> restored properly I'd suggest:
>         1. Call getrlimit()
>         2. Call altstack() in a way rigged to fail
>         3. Call getrlimit() again
>         4. Compare results from (1) and (3)
>
> > I don't really see a need to optimize for changing altstack's
> > implementation.  It's less than a hundred lines of code, and only ten
> tests
> > using setcall.  Can you tell me why you think it's important?
>
> So, the checking of specific calls makes the tests very dependent on
> altstack's implementation, and the framework of macros used to do it
> makes it difficult to change one test without changing them all.
>
> Together, that makes it almost impossible to change anything
> significant about the altstack implementation without having to
> significantly rewrite the tests.  And if the tests are significantly
> rewritten, it's hard to be confident that they still check the things
> they used to.
>
> Which undermines the whole value of a testsuite in allowing you to
> modify the implementation while being reasonably confident you haven't
> changed the desired behaviour.
>
> This is not theoretical; I have a couple of changes in mind for which
> the primary obstacle is adjusting the testsuite to match (switching to
> ccan/coroutine to avoid the x86_64 specific code, and using mprotect()
> instead of MAP_GROWSDOWN+setrlimit()).
>
> > On Fri, Jun 3, 2016 at 4:40 AM David Gibson <david@gibson.dropbear.id.au
> >
> > wrote:
> >
> > > altstack/test/run.c uses some hairy macros to intercept the standard
> > > library functions that altstack uses.  This has two purposes: 1) to
> > > conditionally cause those functions to fail, and thereby test
> altstack's
> > > error paths, and 2) log which of the library functions was called in
> each
> > > testcase.
> > >
> > > The second function isn't actually useful - for the purposes of
> testing the
> > > module, we want to check the actual behaviour, not what calls it made
> in
> > > what order to accomplish it.  Explicitly checking the calls makes it
> much
> > > harder to change altstack's implementation without breaking the tests.
> > >
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  ccan/altstack/test/run.c | 73
> > > ++++++++++++++----------------------------------
> > >  1 file changed, 21 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> > > index e109ccb..091d1f5 100644
> > > --- a/ccan/altstack/test/run.c
> > > +++ b/ccan/altstack/test/run.c
> > > @@ -20,18 +20,17 @@ enum {
> > >         sigaction_      = 1<<4,
> > >         munmap_         = 1<<5,
> > >  };
> > > -int fail, call1, call2;
> > > +int fail;
> > >  char *m_;
> > >  rlim_t msz_;
> > >  #define e(x) (900+(x))
> > >  #define seterr(x) (errno = e(x))
> > > -#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno ||
> > > state.out ? (x) : 0))
> > > -#define getrlimit(...)         (fail&getrlimit_        ?
> > > (seterr(getrlimit_),          -1) : (setcall(getrlimit_),
> > >  getrlimit(__VA_ARGS__)))
> > > -#define mmap(...)              (fail&mmap_             ?
> (seterr(mmap_),
> > >      (void *)-1) : (setcall(mmap_),          mmap(__VA_ARGS__)))
> > > -#define munmap(a, b)           (fail&munmap_           ?
> > > (seterr(munmap_),             -1) : (setcall(munmap_),
> > > munmap(m_=(a), msz_=(b))))
> > > -#define setrlimit(...)         (fail&setrlimit_        ?
> > > (seterr(setrlimit_),          -1) : (setcall(setrlimit_),
> > >  setrlimit(__VA_ARGS__)))
> > > -#define sigaltstack(...)       (fail&sigaltstack_      ?
> > > (seterr(sigaltstack_),        -1) : (setcall(sigaltstack_),
> > >  sigaltstack(__VA_ARGS__)))
> > > -#define sigaction(...)         (fail&sigaction_        ?
> > > (seterr(sigaction_),          -1) : (setcall(sigaction_),
> > >  sigaction(__VA_ARGS__)))
> > > +#define getrlimit(...)         (fail&getrlimit_        ?
> > > (seterr(getrlimit_),          -1) : getrlimit(__VA_ARGS__))
> > > +#define mmap(...)              (fail&mmap_             ?
> (seterr(mmap_),
> > >      (void *)-1) : mmap(__VA_ARGS__))
> > > +#define munmap(a, b)           (fail&munmap_           ?
> > > (seterr(munmap_),             -1) : munmap(m_=(a), msz_=(b)))
> > > +#define setrlimit(...)         (fail&setrlimit_        ?
> > > (seterr(setrlimit_),          -1) : setrlimit(__VA_ARGS__))
> > > +#define sigaltstack(...)       (fail&sigaltstack_      ?
> > > (seterr(sigaltstack_),        -1) : sigaltstack(__VA_ARGS__))
> > > +#define sigaction(...)         (fail&sigaction_        ?
> > > (seterr(sigaction_),          -1) : sigaction(__VA_ARGS__))
> > >
> > >  #define KiB (1024UL)
> > >  #define MiB (KiB*KiB)
> > > @@ -58,74 +57,48 @@ static void *wrap(void *i)
> > >         return wrap;
> > >  }
> > >
> > > -#define chkfail(x, y, z, c1, c2)
>  \
> > > +#define chkfail(x, y, z)
>  \
> > >         do {
>   \
> > > -               call1 = 0;
>   \
> > > -               call2 = 0;
>   \
> > >                 errno = 0;
>   \
> > >                 ok1((fail = x) && (y));
>  \
> > >                 ok1(errno == (z));
>   \
> > > -               ok1(call1 == (c1));
>  \
> > > -               ok1(call2 == (c2));
>  \
> > >         } while (0);
> > >
> > > -#define chkok(y, z, c1, c2)
>   \
> > > +#define chkok(y, z)
>   \
> > >         do {
>   \
> > > -               call1 = 0;
>   \
> > > -               call2 = 0;
>   \
> > >                 errno = 0;
>   \
> > >                 fail = 0;
>  \
> > >                 ok1((y));
>  \
> > >                 ok1(errno == (z));
>   \
> > > -               ok1(call1 == (c1));
>  \
> > > -               ok1(call2 == (c2));
>  \
> > >         } while (0)
> > >
> > >  int main(void)
> > >  {
> > >         long pgsz = sysconf(_SC_PAGESIZE);
> > >
> > > -       plan_tests(50);
> > > +       plan_tests(28);
> > >
> > > -       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(getrlimit_),
> > > -               0,
> > > -               0);
> > > +       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(getrlimit_));
> > >
> > > -       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(setrlimit_),
> > > -               getrlimit_,
> > > -               0);
> > > +       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(setrlimit_));
> > >
> > > -       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(mmap_),
> > > -               getrlimit_|setrlimit_,
> > > -               setrlimit_);
> > > +       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(mmap_));
> > >
> > > -       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(sigaltstack_),
> > > -               getrlimit_|setrlimit_|mmap_,
> > > -               setrlimit_|munmap_);
> > > +       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(sigaltstack_));
> > >
> > > -       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(sigaction_),
> > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_,
> > > -               setrlimit_|munmap_|sigaltstack_);
> > > +       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0),
> NULL) ==
> > > -1, e(sigaction_));
> > >
> > > -       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> > > ==  1, e(munmap_),
> > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > -               setrlimit_|sigaltstack_|sigaction_);
> > > +       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> > > ==  1, e(munmap_));
> > >         if (fail = 0, munmap(m_, msz_) == -1)
> > >                 err(1, "munmap");
> > >
> > > -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW,
> > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW);
> > >
> > >         // be sure segv catch is repeatable (SA_NODEFER)
> > > -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW,
> > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW);
> > >
> > >         used = 1;
> > > -       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW,
> > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > -               setrlimit_|sigaltstack_|sigaction_);
> > > +       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW);
> > >         if (fail = 0, munmap(m_, msz_) == -1)
> > >                 err(1, "munmap");
> > >
> > > @@ -150,17 +123,13 @@ int main(void)
> > >         ok1(strcmp(buf, estr "\n") == 0);
> > >
> > >         used = 1;
> > > -       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW,
> > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > +       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> > > NULL) == -1, EOVERFLOW);
> > >
> > >         diag("used: %lu", used);
> > >         ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
> > >
> > >         used = 0;
> > > -       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> > > NULL) == 0, 0,
> > > -
> > >  getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
> > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > +       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> > > NULL) == 0, 0);
> > >
> > >         used = 1;
> > >         altstack_rsp_save();
> > >
> > >
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_
> _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

[-- Attachment #1.2: Type: text/html, Size: 16510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 5/5] altstack: Don't log internal calls in test cases

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 12638 bytes --]

On Thu, Jun 16, 2016 at 08:45:50PM +0000, Dan Good wrote:
> Very well, I've applied your patches as provided (except I dropped the
> trailing underscore from state.rsp_save).

Cool, thanks.  I'll follow up with the more substantial changes I had
in mind when I get the chance.

> 
> On Thu, Jun 16, 2016 at 7:12 AM David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > On Sun, Jun 12, 2016 at 02:10:18AM +0000, Dan Good wrote:
> > > Hairy macros?  From the author of the cppmagic module, I shall take that
> > as
> > > a compliment.
> >
> > Touché.
> >
> > That said, cppmagic is using hairy magic to do some things that are,
> > as far as I know, impossible by any other method.  I don't think
> > there's a similar justification here.
> >
> > > The purpose of the setcall macro and related checks is ensure the
> > > correctness of the error path, i.e. if setrlimit ran before a failure, it
> > > must run again to undo the first; if mmap ran before a failure, munmap
> > must
> > > run after.  I find it very reassuring to know these tests exist and pass.
> > > Do you see no value in that?
> >
> > So, there's certainly value in checking the error paths, but doing it
> > by checking what calls the implementation makes is not a great way of
> > doing it.  It's pretty subject to both false positives and false
> > negatives.
> >
> > What I'd suggest instead is actually checking the behaviour in
> > question.  For example, if you want to check that the rlimit is
> > restored properly I'd suggest:
> >         1. Call getrlimit()
> >         2. Call altstack() in a way rigged to fail
> >         3. Call getrlimit() again
> >         4. Compare results from (1) and (3)
> >
> > > I don't really see a need to optimize for changing altstack's
> > > implementation.  It's less than a hundred lines of code, and only ten
> > tests
> > > using setcall.  Can you tell me why you think it's important?
> >
> > So, the checking of specific calls makes the tests very dependent on
> > altstack's implementation, and the framework of macros used to do it
> > makes it difficult to change one test without changing them all.
> >
> > Together, that makes it almost impossible to change anything
> > significant about the altstack implementation without having to
> > significantly rewrite the tests.  And if the tests are significantly
> > rewritten, it's hard to be confident that they still check the things
> > they used to.
> >
> > Which undermines the whole value of a testsuite in allowing you to
> > modify the implementation while being reasonably confident you haven't
> > changed the desired behaviour.
> >
> > This is not theoretical; I have a couple of changes in mind for which
> > the primary obstacle is adjusting the testsuite to match (switching to
> > ccan/coroutine to avoid the x86_64 specific code, and using mprotect()
> > instead of MAP_GROWSDOWN+setrlimit()).
> >
> > > On Fri, Jun 3, 2016 at 4:40 AM David Gibson <david@gibson.dropbear.id.au
> > >
> > > wrote:
> > >
> > > > altstack/test/run.c uses some hairy macros to intercept the standard
> > > > library functions that altstack uses.  This has two purposes: 1) to
> > > > conditionally cause those functions to fail, and thereby test
> > altstack's
> > > > error paths, and 2) log which of the library functions was called in
> > each
> > > > testcase.
> > > >
> > > > The second function isn't actually useful - for the purposes of
> > testing the
> > > > module, we want to check the actual behaviour, not what calls it made
> > in
> > > > what order to accomplish it.  Explicitly checking the calls makes it
> > much
> > > > harder to change altstack's implementation without breaking the tests.
> > > >
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  ccan/altstack/test/run.c | 73
> > > > ++++++++++++++----------------------------------
> > > >  1 file changed, 21 insertions(+), 52 deletions(-)
> > > >
> > > > diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
> > > > index e109ccb..091d1f5 100644
> > > > --- a/ccan/altstack/test/run.c
> > > > +++ b/ccan/altstack/test/run.c
> > > > @@ -20,18 +20,17 @@ enum {
> > > >         sigaction_      = 1<<4,
> > > >         munmap_         = 1<<5,
> > > >  };
> > > > -int fail, call1, call2;
> > > > +int fail;
> > > >  char *m_;
> > > >  rlim_t msz_;
> > > >  #define e(x) (900+(x))
> > > >  #define seterr(x) (errno = e(x))
> > > > -#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno ||
> > > > state.out ? (x) : 0))
> > > > -#define getrlimit(...)         (fail&getrlimit_        ?
> > > > (seterr(getrlimit_),          -1) : (setcall(getrlimit_),
> > > >  getrlimit(__VA_ARGS__)))
> > > > -#define mmap(...)              (fail&mmap_             ?
> > (seterr(mmap_),
> > > >      (void *)-1) : (setcall(mmap_),          mmap(__VA_ARGS__)))
> > > > -#define munmap(a, b)           (fail&munmap_           ?
> > > > (seterr(munmap_),             -1) : (setcall(munmap_),
> > > > munmap(m_=(a), msz_=(b))))
> > > > -#define setrlimit(...)         (fail&setrlimit_        ?
> > > > (seterr(setrlimit_),          -1) : (setcall(setrlimit_),
> > > >  setrlimit(__VA_ARGS__)))
> > > > -#define sigaltstack(...)       (fail&sigaltstack_      ?
> > > > (seterr(sigaltstack_),        -1) : (setcall(sigaltstack_),
> > > >  sigaltstack(__VA_ARGS__)))
> > > > -#define sigaction(...)         (fail&sigaction_        ?
> > > > (seterr(sigaction_),          -1) : (setcall(sigaction_),
> > > >  sigaction(__VA_ARGS__)))
> > > > +#define getrlimit(...)         (fail&getrlimit_        ?
> > > > (seterr(getrlimit_),          -1) : getrlimit(__VA_ARGS__))
> > > > +#define mmap(...)              (fail&mmap_             ?
> > (seterr(mmap_),
> > > >      (void *)-1) : mmap(__VA_ARGS__))
> > > > +#define munmap(a, b)           (fail&munmap_           ?
> > > > (seterr(munmap_),             -1) : munmap(m_=(a), msz_=(b)))
> > > > +#define setrlimit(...)         (fail&setrlimit_        ?
> > > > (seterr(setrlimit_),          -1) : setrlimit(__VA_ARGS__))
> > > > +#define sigaltstack(...)       (fail&sigaltstack_      ?
> > > > (seterr(sigaltstack_),        -1) : sigaltstack(__VA_ARGS__))
> > > > +#define sigaction(...)         (fail&sigaction_        ?
> > > > (seterr(sigaction_),          -1) : sigaction(__VA_ARGS__))
> > > >
> > > >  #define KiB (1024UL)
> > > >  #define MiB (KiB*KiB)
> > > > @@ -58,74 +57,48 @@ static void *wrap(void *i)
> > > >         return wrap;
> > > >  }
> > > >
> > > > -#define chkfail(x, y, z, c1, c2)
> >  \
> > > > +#define chkfail(x, y, z)
> >  \
> > > >         do {
> >   \
> > > > -               call1 = 0;
> >   \
> > > > -               call2 = 0;
> >   \
> > > >                 errno = 0;
> >   \
> > > >                 ok1((fail = x) && (y));
> >  \
> > > >                 ok1(errno == (z));
> >   \
> > > > -               ok1(call1 == (c1));
> >  \
> > > > -               ok1(call2 == (c2));
> >  \
> > > >         } while (0);
> > > >
> > > > -#define chkok(y, z, c1, c2)
> >   \
> > > > +#define chkok(y, z)
> >   \
> > > >         do {
> >   \
> > > > -               call1 = 0;
> >   \
> > > > -               call2 = 0;
> >   \
> > > >                 errno = 0;
> >   \
> > > >                 fail = 0;
> >  \
> > > >                 ok1((y));
> >  \
> > > >                 ok1(errno == (z));
> >   \
> > > > -               ok1(call1 == (c1));
> >  \
> > > > -               ok1(call2 == (c2));
> >  \
> > > >         } while (0)
> > > >
> > > >  int main(void)
> > > >  {
> > > >         long pgsz = sysconf(_SC_PAGESIZE);
> > > >
> > > > -       plan_tests(50);
> > > > +       plan_tests(28);
> > > >
> > > > -       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(getrlimit_),
> > > > -               0,
> > > > -               0);
> > > > +       chkfail(getrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(getrlimit_));
> > > >
> > > > -       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(setrlimit_),
> > > > -               getrlimit_,
> > > > -               0);
> > > > +       chkfail(setrlimit_,     altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(setrlimit_));
> > > >
> > > > -       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(mmap_),
> > > > -               getrlimit_|setrlimit_,
> > > > -               setrlimit_);
> > > > +       chkfail(mmap_,          altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(mmap_));
> > > >
> > > > -       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(sigaltstack_),
> > > > -               getrlimit_|setrlimit_|mmap_,
> > > > -               setrlimit_|munmap_);
> > > > +       chkfail(sigaltstack_,   altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(sigaltstack_));
> > > >
> > > > -       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(sigaction_),
> > > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_,
> > > > -               setrlimit_|munmap_|sigaltstack_);
> > > > +       chkfail(sigaction_,     altstack(8*MiB, wrap, int2ptr(0),
> > NULL) ==
> > > > -1, e(sigaction_));
> > > >
> > > > -       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> > > > ==  1, e(munmap_),
> > > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > -               setrlimit_|sigaltstack_|sigaction_);
> > > > +       chkfail(munmap_,        altstack(8*MiB, wrap, int2ptr(0), NULL)
> > > > ==  1, e(munmap_));
> > > >         if (fail = 0, munmap(m_, msz_) == -1)
> > > >                 err(1, "munmap");
> > > >
> > > > -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW,
> > > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > > +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW);
> > > >
> > > >         // be sure segv catch is repeatable (SA_NODEFER)
> > > > -       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW,
> > > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > > +       chkok(                  altstack(1*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW);
> > > >
> > > >         used = 1;
> > > > -       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW,
> > > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > -               setrlimit_|sigaltstack_|sigaction_);
> > > > +       chkfail(munmap_,        altstack(1*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW);
> > > >         if (fail = 0, munmap(m_, msz_) == -1)
> > > >                 err(1, "munmap");
> > > >
> > > > @@ -150,17 +123,13 @@ int main(void)
> > > >         ok1(strcmp(buf, estr "\n") == 0);
> > > >
> > > >         used = 1;
> > > > -       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW,
> > > > -               getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
> > > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > > +       chkok(                  altstack(8*MiB, wrap, int2ptr(1000000),
> > > > NULL) == -1, EOVERFLOW);
> > > >
> > > >         diag("used: %lu", used);
> > > >         ok1(used >= 8*MiB - pgsz && used <= 8*MiB + pgsz);
> > > >
> > > >         used = 0;
> > > > -       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> > > > NULL) == 0, 0,
> > > > -
> > > >  getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,
> > > > -               setrlimit_|munmap_|sigaltstack_|sigaction_);
> > > > +       chkok(                  altstack(8*MiB, wrap, int2ptr(100000),
> > > > NULL) == 0, 0);
> > > >
> > > >         used = 1;
> > > >         altstack_rsp_save();
> > > >
> > > >
> >
> >

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

Re: [PATCH 0/5] altstack: cleanups

[Dan Good]

[-- Attachment #1.1: Type: text/plain, Size: 1021 bytes --]

David, thank you for improving the code. I'm traveling for the next week
with only an ipad. I'd like to ask your thoughts on a topic or two, but
typing on this is grueling. I hope to try for a longer reply later. Thanks
again. -Dan

On Fri, Jun 3, 2016 at 4:40 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> Dan,
>
> Here are a bunch of assorted cleanups to the altstack module.  If they
> seem reasonable to you, please apply.
>
> David Gibson (5):
>   altstack: Consolidate thread-local variables
>   altstack: Restore alternate signal stack state
>   altstack: Use ptrint instead of bare casts
>   altstack: Don't use 0 pointer literals
>   altstack: Don't log internal calls in test cases
>
>  ccan/altstack/_info      |  9 ++++--
>  ccan/altstack/altstack.c | 78
> +++++++++++++++++++++++++-----------------------
>  ccan/altstack/altstack.h |  6 ++--
>  ccan/altstack/test/run.c | 76
> ++++++++++++++--------------------------------
>  4 files changed, 74 insertions(+), 95 deletions(-)
>
> --
> 2.5.5
>
>

[-- Attachment #1.2: Type: text/html, Size: 1388 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan