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