Re: [PATCH 5/5] altstack: Don't log internal calls in test cases

David Gibson <[email protected]> Thu, 16 Jun 2016 17:34:49 +1000
Newsgroups	org.ozlabs.lists.ccan
Message-ID	<[email protected]>
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 <[email protected]>
> 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 <[email protected]>
> > ---
> >  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
[email protected]
https://lists.ozlabs.org/listinfo/ccan
signature.asc (application/pgp-signature, 819 B)