From: David Gibson <david@gibson.dropbear.id.au>
To: Dan Good <dan@dancancode.com>
Cc: ccan@lists.ozlabs.org
Subject: Re: [PATCH 5/5] altstack: Don't log internal calls in test cases
Date: Mon, 20 Jun 2016 18:26:21 +1000 [thread overview]
Message-ID: <20160620082621.GK6858@voom.fritz.box> (raw)
In-Reply-To: <CACNkOJPf9-HtrOMPsnkzjv59nOkeoH5WXC2e6b4n=asBNisbyw@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 12638 bytes --]
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@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@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@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
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan
next prev parent reply other threads:[~2016-06-20 12:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-03 8:41 [PATCH 0/5] altstack: cleanups David Gibson
2016-06-03 8:41 ` [PATCH 1/5] altstack: Consolidate thread-local variables David Gibson
2016-06-03 8:42 ` [PATCH 2/5] altstack: Restore alternate signal stack state David Gibson
2016-06-03 8:42 ` [PATCH 3/5] altstack: Use ptrint instead of bare casts David Gibson
2016-06-03 8:42 ` [PATCH 4/5] altstack: Don't use 0 pointer literals David Gibson
2016-06-12 1:27 ` Dan Good
2016-06-14 4:15 ` Rusty Russell
2016-06-16 6:45 ` David Gibson
2016-06-16 20:38 ` Dan Good
2016-06-03 8:42 ` [PATCH 5/5] altstack: Don't log internal calls in test cases David Gibson
2016-06-12 2:10 ` Dan Good
2016-06-16 7:34 ` David Gibson
2016-06-16 20:45 ` Dan Good
2016-06-20 8:26 ` David Gibson [this message]
2016-06-03 23:29 ` [PATCH 0/5] altstack: cleanups Dan Good
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160620082621.GK6858@voom.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=ccan@lists.ozlabs.org \
--cc=dan@dancancode.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).