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