[ccan] [PATCH 5/5] altstack: Don't log internal calls in test cases
Dan Good
dan at dancancode.com
Fri Jun 17 06:45:50 AEST 2016
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 <david at 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 <david at 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 <david at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160616/d61f4fab/attachment-0001.html>
More information about the ccan
mailing list