[ccan] [PATCH 5/5] altstack: Don't log internal calls in test cases
David Gibson
david at gibson.dropbear.id.au
Mon Jun 20 18:26:21 AEST 2016
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 <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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160620/1ee95029/attachment.sig>
More information about the ccan
mailing list