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