From: Dan Good <dan@dancancode.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: ccan@lists.ozlabs.org
Subject: Re: [PATCH 5/5] altstack: Don't log internal calls in test cases
Date: Sun, 12 Jun 2016 02:10:18 +0000 [thread overview]
Message-ID: <CACNkOJPSMffBBHiS=ti6-TdQRrvvoTGqENX1hqgpJAxYxGYnQQ@mail.gmail.com> (raw)
In-Reply-To: <1464943323-8225-6-git-send-email-david@gibson.dropbear.id.au>
[-- Attachment #1.1: Type: text/plain, Size: 9382 bytes --]
Hairy macros? From the author of the cppmagic module, I shall take that as
a compliment.
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?
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?
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();
> --
> 2.5.5
>
>
[-- Attachment #1.2: Type: text/html, Size: 11651 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-12 2:10 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 [this message]
2016-06-16 7:34 ` David Gibson
2016-06-16 20:45 ` Dan Good
2016-06-20 8:26 ` David Gibson
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='CACNkOJPSMffBBHiS=ti6-TdQRrvvoTGqENX1hqgpJAxYxGYnQQ@mail.gmail.com' \
--to=dan@dancancode.com \
--cc=ccan@lists.ozlabs.org \
--cc=david@gibson.dropbear.id.au \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.