Re: [PATCH] Fix p{readv,writev}{64} consolidation implementation

[Mike Frysinger <vapier at gentoo dot org>]

On 15 Jun 2016 08:50, Florian Weimer wrote:
> On 06/15/2016 07:37 AM, Mike Frysinger wrote:
> >> +#define FAIL() \
> >> +  do { printf ("error: failure on line %d\n", __LINE__); return 1; } while (0)
> >> ...
> >> +  ret = pwritev (temp_fd, iov, 2, 0);
> >> +  if (ret == -1)
> >> +    FAIL ();
> >
> > as a personal stance, never been a big fan of messages that just have a
> > line number.  when you get reports/logs, you end having to chase down the
> > exact same source tree as the reporter.
> 
> At least there is a line number.  Many existing tests do not even have that.
> 
> > why can't we just assert() everywhere ?  we rely on that in tests already
> > and we wouldn't have to do any checking otherwise.
> >   ret = pwritev (temp_fd, iov, 2, 0);
> >   assert (ret == sizeof buf1 + sizeof buf2);
> 
> assert writes to standard error, not standard output, where it will be 
> captured.  I don't know why we don't capture both.

we keep running into this issue.  rather than try to be super vigilent
all the time, why don't we simply:

--- a/test-skeleton.c
+++ b/test-skeleton.c
@@ -343,6 +343,10 @@ main (int argc, char *argv[])
   setbuf (stdout, NULL);
 #endif
 
+  fclose (stderr);
+  dup2 (STDOUT_FILENO, STDERR_FILENO);
+  stderr = fdopen (STDERR_FILENO, "w");
+
   while ((opt = getopt_long (argc, argv, "+", options, NULL)) != -1)
     switch (opt)
       {

> There seems to be a policy against tests aborting on failure, too, but 
> we are not very consistent about that.  In my experience, people who 
> build and test glibc are more likely to shrug off tests with non-zero 
> exit status than to ignore tests with aborts, although they really 
> should treat them the same.

we've actively been adding tests with abort() usage.  i'm not sure there
is a policy against it.

i'm not sure i agree on the non-zero-vs-abort issue, but i'm not everyone.
i look at all tests that fail regardless of reason.

> I think we should really have some sort of libtest.a, which provides 
> test helpers, error-checking functions such as xmalloc, and general 
> helpers for setting up special test environments.  I'm a bit worried 
> about figuring out the proper dependencies, so that libtest.a is built 
> before all the tests are linked.

isn't this what test-skeleton.c does for us now ?  and we all agree that
all tests should be using that.

the only downside is the inclusion ordering ... a lot of tests do either:
(1) at top (allows helper funcs to be used in the test)
static int do_test (void);
#define TEST_FUNCTION do_test ()
#include "../test-skeleton.c"
(2) at bottom (does not allow access to helper funcs)
#define TEST_FUNCTION do_test ()
#include "../test-skeleton.c"

imo we should just mandate all tests use the entry point "do_test" and it
take argc/argv args (even if they're unused), and then all tests can pull
test-skeleton.c in at the top.
-mike

Attachment: signature.asc
Description: Digital signature