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

[Dan Good]

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 
wrote:

> On Sun, Jun 12, 2016 at 02:10:18AM +, 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  >
> > 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 
> > > ---
> > >  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_?
> > > (seterr(getrlimit_),  -1) : (setcall(getrlimit_),
> > >  getrlimit(__VA_ARGS__)))
> > > -#define mmap(...)  (fail_ ?
> (seterr(mmap_),
> > >  (void *)-1) : (setcall(mmap_),  mmap(__VA_ARGS__)))
> > > -#define munmap(a, b)   (fail_   ?
> > > (seterr(munmap_), -1) : (setcall(munmap_),
> > > munmap(m_=(a), msz_=(b
> > > -#define setrlimit(...) (fail_?
> > > (seterr(setrlimit_),  -1) : (setcall(setrlimit_),
> > >  setrlimit(__VA_ARGS__)))
> > > -#define sigaltstack(...)   (fail_  ?
> > > (seterr(sigaltstack_),-1) : (setcall(sigaltstack_),
> > >  sigaltstack(__VA_ARGS__)))
> > > -#define sigaction(...) (fail_?
> > > (seterr(sigaction_),  -1) 

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

[Dan Good]

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 
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 
> ---
>  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_?
> (seterr(getrlimit_),  -1) : (setcall(getrlimit_),
>  getrlimit(__VA_ARGS__)))
> -#define mmap(...)  (fail_ ? (seterr(mmap_),
>  (void *)-1) : (setcall(mmap_),  mmap(__VA_ARGS__)))
> -#define munmap(a, b)   (fail_   ?
> (seterr(munmap_), -1) : (setcall(munmap_),
> munmap(m_=(a), msz_=(b
> -#define setrlimit(...) (fail_?
> (seterr(setrlimit_),  -1) : (setcall(setrlimit_),
>  setrlimit(__VA_ARGS__)))
> -#define sigaltstack(...)   (fail_  ?
> (seterr(sigaltstack_),-1) : (setcall(sigaltstack_),
>  sigaltstack(__VA_ARGS__)))
> -#define sigaction(...) (fail_?
> (seterr(sigaction_),  -1) : (setcall(sigaction_),
>  sigaction(__VA_ARGS__)))
> +#define getrlimit(...) (fail_?
> (seterr(getrlimit_),  -1) : getrlimit(__VA_ARGS__))
> +#define mmap(...)  (fail_ ? (seterr(mmap_),
>  (void *)-1) : mmap(__VA_ARGS__))
> +#define munmap(a, b)   (fail_   ?
> (seterr(munmap_), -1) : munmap(m_=(a), msz_=(b)))
> +#define setrlimit(...) (fail_?
> (seterr(setrlimit_),  -1) : setrlimit(__VA_ARGS__))
> +#define sigaltstack(...)   (fail_  ?
> (seterr(sigaltstack_),-1) : sigaltstack(__VA_ARGS__))
> +#define sigaction(...) (fail_?
> (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));   

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

[David Gibson]

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 
---
 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_? (seterr(getrlimit_),  
-1) : (setcall(getrlimit_), getrlimit(__VA_ARGS__)))
-#define mmap(...)  (fail_ ? (seterr(mmap_),   
(void *)-1) : (setcall(mmap_),  mmap(__VA_ARGS__)))
-#define munmap(a, b)   (fail_   ? (seterr(munmap_), 
-1) : (setcall(munmap_),munmap(m_=(a), msz_=(b
-#define setrlimit(...) (fail_? (seterr(setrlimit_),  
-1) : (setcall(setrlimit_), setrlimit(__VA_ARGS__)))
-#define sigaltstack(...)   (fail_  ? 
(seterr(sigaltstack_),-1) : (setcall(sigaltstack_),   
sigaltstack(__VA_ARGS__)))
-#define sigaction(...) (fail_? (seterr(sigaction_),  
-1) : (setcall(sigaction_), sigaction(__VA_ARGS__)))
+#define getrlimit(...) (fail_? (seterr(getrlimit_),  
-1) : getrlimit(__VA_ARGS__))
+#define mmap(...)  (fail_ ? (seterr(mmap_),   
(void *)-1) : mmap(__VA_ARGS__))
+#define munmap(a, b)   (fail_   ? (seterr(munmap_), 
-1) : munmap(m_=(a), msz_=(b)))
+#define setrlimit(...) (fail_? (seterr(setrlimit_),  
-1) : setrlimit(__VA_ARGS__))
+#define sigaltstack(...)   (fail_  ? 
(seterr(sigaltstack_),-1) : sigaltstack(__VA_ARGS__))
+#define sigaction(...) (fail_? (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,