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