Re: [ccan] [PATCH 0/5] altstack: cleanups

[Dan Good]

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 
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
>
>
___
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan

[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 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -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 *) 100, 0) == 
-1, EOVERFLOW,
+   chkok(  altstack(1*MiB, wrap, int2ptr(100), 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 *) 100, 0) == 
-1, EOVERFLOW,
+   chkok(  altstack(1*MiB, wrap, int2ptr(100), 0) == 
-1, EOVERFLOW,
getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,
setrlimit_|munmap_|sigaltstack_|sigaction_);
 
used = 1;
-   chkfail(munmap_,altstack(1*MiB, wrap, (void *) 100, 0) == 
-1, EOVERFLOW,
+   chkfail(munmap_,altstack(1*MiB, wrap, int2ptr(100), 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 *) 100, 0) == 
-1, EOVERFLOW,
+   chkok(  altstack(8*MiB, wrap, int2ptr(100), 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 *) 10, 0) == 0, 
0,
+   chkok(  altstack(8*MiB, wrap, int2ptr(10), 0) == 0, 
0,
getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,

[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 
---
 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_? (seterr(getrlimit_),  
-1) : (setcall(getrlimit_), getrlimit(__VA_ARGS__)))
-#define mmap(...)  (fail_ ? (seterr(mmap_),   
(void *)-1) : (setcall(mmap_),  mmap(__VA_ARGS__)))
-#define munmap(a, b)   (fail_   ? (seterr(munmap_), 
-1) : (setcall(munmap_),munmap(m_=(a), msz_=(b
-#define setrlimit(...) (fail_? (seterr(setrlimit_),  
-1) : (setcall(setrlimit_), setrlimit(__VA_ARGS__)))
-#define sigaltstack(...)   (fail_  ? 
(seterr(sigaltstack_),-1) : (setcall(sigaltstack_),   
sigaltstack(__VA_ARGS__)))
-#define sigaction(...) (fail_? (seterr(sigaction_),  
-1) : (setcall(sigaction_), sigaction(__VA_ARGS__)))
+#define getrlimit(...) (fail_? (seterr(getrlimit_),  
-1) : getrlimit(__VA_ARGS__))
+#define mmap(...)  (fail_ ? (seterr(mmap_),   
(void *)-1) : mmap(__VA_ARGS__))
+#define munmap(a, b)   (fail_   ? (seterr(munmap_), 
-1) : munmap(m_=(a), msz_=(b)))
+#define setrlimit(...) (fail_? (seterr(setrlimit_),  
-1) : setrlimit(__VA_ARGS__))
+#define sigaltstack(...)   (fail_  ? 
(seterr(sigaltstack_),-1) : sigaltstack(__VA_ARGS__))
+#define sigaction(...) (fail_? (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, 

[ccan] [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