<div dir="ltr">Very well, I've applied your patches as provided (except I dropped the trailing underscore from state.rsp_save).<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 16, 2016 at 7:12 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">On Sun, Jun 12, 2016 at 02:10:18AM +0000, Dan Good wrote:<br>
> Hairy macros? From the author of the cppmagic module, I shall take that as<br>
> a compliment.<br>
<br>
Touché.<br>
<br>
That said, cppmagic is using hairy magic to do some things that are,<br>
as far as I know, impossible by any other method. I don't think<br>
there's a similar justification here.<br>
<br>
> The purpose of the setcall macro and related checks is ensure the<br>
> correctness of the error path, i.e. if setrlimit ran before a failure, it<br>
> must run again to undo the first; if mmap ran before a failure, munmap must<br>
> run after. I find it very reassuring to know these tests exist and pass.<br>
> Do you see no value in that?<br>
<br>
So, there's certainly value in checking the error paths, but doing it<br>
by checking what calls the implementation makes is not a great way of<br>
doing it. It's pretty subject to both false positives and false<br>
negatives.<br>
<br>
What I'd suggest instead is actually checking the behaviour in<br>
question. For example, if you want to check that the rlimit is<br>
restored properly I'd suggest:<br>
1. Call getrlimit()<br>
2. Call altstack() in a way rigged to fail<br>
3. Call getrlimit() again<br>
4. Compare results from (1) and (3)<br>
<br>
> I don't really see a need to optimize for changing altstack's<br>
> implementation. It's less than a hundred lines of code, and only ten tests<br>
> using setcall. Can you tell me why you think it's important?<br>
<br>
So, the checking of specific calls makes the tests very dependent on<br>
altstack's implementation, and the framework of macros used to do it<br>
makes it difficult to change one test without changing them all.<br>
<br>
Together, that makes it almost impossible to change anything<br>
significant about the altstack implementation without having to<br>
significantly rewrite the tests. And if the tests are significantly<br>
rewritten, it's hard to be confident that they still check the things<br>
they used to.<br>
<br>
Which undermines the whole value of a testsuite in allowing you to<br>
modify the implementation while being reasonably confident you haven't<br>
changed the desired behaviour.<br>
<br>
This is not theoretical; I have a couple of changes in mind for which<br>
the primary obstacle is adjusting the testsuite to match (switching to<br>
ccan/coroutine to avoid the x86_64 specific code, and using mprotect()<br>
instead of MAP_GROWSDOWN+setrlimit()).<br>
<br>
> On Fri, Jun 3, 2016 at 4:40 AM David Gibson <<a href="mailto:david@gibson.dropbear.id.au" target="_blank">david@gibson.dropbear.id.au</a>><br>
> wrote:<br>
><br>
> > 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>
> > ++++++++++++++----------------------------------<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 ||<br>
> > state.out ? (x) : 0))<br>
> > -#define getrlimit(...) (fail&getrlimit_ ?<br>
> > (seterr(getrlimit_), -1) : (setcall(getrlimit_),<br>
> > getrlimit(__VA_ARGS__)))<br>
> > -#define mmap(...) (fail&mmap_ ? (seterr(mmap_),<br>
> > (void *)-1) : (setcall(mmap_), mmap(__VA_ARGS__)))<br>
> > -#define munmap(a, b) (fail&munmap_ ?<br>
> > (seterr(munmap_), -1) : (setcall(munmap_),<br>
> > munmap(m_=(a), msz_=(b))))<br>
> > -#define setrlimit(...) (fail&setrlimit_ ?<br>
> > (seterr(setrlimit_), -1) : (setcall(setrlimit_),<br>
> > setrlimit(__VA_ARGS__)))<br>
> > -#define sigaltstack(...) (fail&sigaltstack_ ?<br>
> > (seterr(sigaltstack_), -1) : (setcall(sigaltstack_),<br>
> > sigaltstack(__VA_ARGS__)))<br>
> > -#define sigaction(...) (fail&sigaction_ ?<br>
> > (seterr(sigaction_), -1) : (setcall(sigaction_),<br>
> > sigaction(__VA_ARGS__)))<br>
> > +#define getrlimit(...) (fail&getrlimit_ ?<br>
> > (seterr(getrlimit_), -1) : getrlimit(__VA_ARGS__))<br>
> > +#define mmap(...) (fail&mmap_ ? (seterr(mmap_),<br>
> > (void *)-1) : mmap(__VA_ARGS__))<br>
> > +#define munmap(a, b) (fail&munmap_ ?<br>
> > (seterr(munmap_), -1) : munmap(m_=(a), msz_=(b)))<br>
> > +#define setrlimit(...) (fail&setrlimit_ ?<br>
> > (seterr(setrlimit_), -1) : setrlimit(__VA_ARGS__))<br>
> > +#define sigaltstack(...) (fail&sigaltstack_ ?<br>
> > (seterr(sigaltstack_), -1) : sigaltstack(__VA_ARGS__))<br>
> > +#define sigaction(...) (fail&sigaction_ ?<br>
> > (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) ==<br>
> > -1, e(getrlimit_),<br>
> > - 0,<br>
> > - 0);<br>
> > + chkfail(getrlimit_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(getrlimit_));<br>
> ><br>
> > - chkfail(setrlimit_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(setrlimit_),<br>
> > - getrlimit_,<br>
> > - 0);<br>
> > + chkfail(setrlimit_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(setrlimit_));<br>
> ><br>
> > - chkfail(mmap_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(mmap_),<br>
> > - getrlimit_|setrlimit_,<br>
> > - setrlimit_);<br>
> > + chkfail(mmap_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(mmap_));<br>
> ><br>
> > - chkfail(sigaltstack_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(sigaltstack_),<br>
> > - getrlimit_|setrlimit_|mmap_,<br>
> > - setrlimit_|munmap_);<br>
> > + chkfail(sigaltstack_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(sigaltstack_));<br>
> ><br>
> > - chkfail(sigaction_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(sigaction_),<br>
> > - getrlimit_|setrlimit_|mmap_|sigaltstack_,<br>
> > - setrlimit_|munmap_|sigaltstack_);<br>
> > + chkfail(sigaction_, altstack(8*MiB, wrap, int2ptr(0), NULL) ==<br>
> > -1, e(sigaction_));<br>
> ><br>
> > - chkfail(munmap_, altstack(8*MiB, wrap, int2ptr(0), NULL)<br>
> > == 1, e(munmap_),<br>
> > - getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > - setrlimit_|sigaltstack_|sigaction_);<br>
> > + chkfail(munmap_, altstack(8*MiB, wrap, int2ptr(0), NULL)<br>
> > == 1, e(munmap_));<br>
> > if (fail = 0, munmap(m_, msz_) == -1)<br>
> > err(1, "munmap");<br>
> ><br>
> > - chkok( altstack(1*MiB, wrap, int2ptr(1000000),<br>
> > NULL) == -1, EOVERFLOW,<br>
> > - getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > - setrlimit_|munmap_|sigaltstack_|sigaction_);<br>
> > + chkok( altstack(1*MiB, wrap, int2ptr(1000000),<br>
> > NULL) == -1, EOVERFLOW);<br>
> ><br>
> > // be sure segv catch is repeatable (SA_NODEFER)<br>
> > - chkok( altstack(1*MiB, wrap, int2ptr(1000000),<br>
> > NULL) == -1, EOVERFLOW,<br>
> > - getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > - setrlimit_|munmap_|sigaltstack_|sigaction_);<br>
> > + chkok( altstack(1*MiB, wrap, int2ptr(1000000),<br>
> > NULL) == -1, EOVERFLOW);<br>
> ><br>
> > used = 1;<br>
> > - chkfail(munmap_, altstack(1*MiB, wrap, int2ptr(1000000),<br>
> > NULL) == -1, EOVERFLOW,<br>
> > - getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > - setrlimit_|sigaltstack_|sigaction_);<br>
> > + chkfail(munmap_, altstack(1*MiB, wrap, int2ptr(1000000),<br>
> > 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),<br>
> > NULL) == -1, EOVERFLOW,<br>
> > - getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_,<br>
> > - setrlimit_|munmap_|sigaltstack_|sigaction_);<br>
> > + chkok( altstack(8*MiB, wrap, int2ptr(1000000),<br>
> > 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),<br>
> > NULL) == 0, 0,<br>
> > -<br>
> > getrlimit_|setrlimit_|mmap_|sigaltstack_|sigaction_|munmap_,<br>
> > - setrlimit_|munmap_|sigaltstack_|sigaction_);<br>
> > + chkok( altstack(8*MiB, wrap, int2ptr(100000),<br>
> > NULL) == 0, 0);<br>
> ><br>
> > used = 1;<br>
> > altstack_rsp_save();<br>
> ><br>
> ><br>
<br>
--<br>
David Gibson | I'll have my music baroque, and my code<br>
david AT <a href="http://gibson.dropbear.id.au" rel="noreferrer" target="_blank">gibson.dropbear.id.au</a> | minimalist, thank you. NOT _the_ _other_<br>
| _way_ _around_!<br>
<a href="http://www.ozlabs.org/~dgibson" rel="noreferrer" target="_blank">http://www.ozlabs.org/~dgibson</a><br>
</blockquote></div></div>