Re: [PATCH v6 6/6] linux ttyname and ttyname_r: Add tests

[Luke Shumaker <lukeshu at lukeshu dot com>]

On Sat, 11 Nov 2017 19:10:41 -0500,
Christian Brauner wrote:
> On Fri, Nov 10, 2017 at 03:08:27PM -0500, Luke Shumaker wrote:
> Some variable declarations in the middle-of-the stack I pointed out before are
> still left. I don't see this coding style used in the codebase a lot but if
> people don't care and are fine with this I don't care.

(yeah, I was really lazy when looking for those)

Looking around, it isn't common, but it's there and presumably
permissible (example: support/support_test_main.c).  It wasn't allowed
in <= ISO C 90; I figure the reason it's not common in glibc is
because of all of the code written before availability of C99 was to
be relied on.

Since I'll be submitting v7 anyway to address Florian Weimer's
concerns, do you want me to change this?  It's not terrible, but I do
think that it hurts readability of some of the code.

> > +  /* There are 3 groups of tests here.  The first group fairly
> > +     generically does things known to mess up ttyname, and verifies
> > +     that ttyname copes correctly.  The remaining groups are
> > +     increasingly convoluted, as we target specific parts of ttyname
> > +     to try to confuse.  */
> > +
> > +  /* Basic tests that it doesn't get confused by multiple devpts
> > +     instances.  */
> > +  {
> > +    VERIFY (stat (slavename, &st) < 0); /* sanity check */
> > +    if (!doit (slave, "no conflict, no match",
> > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}))
> > +      ok = false;
> 
> I mean these are just test so it's not super crucial in terms of coding style
> but this functions would be well-served by a single goto-based exit point.

The reason I didn't do that is because even if one case fails, I still
want the remainder to run.  (If that weren't true, I'd probably have
just called FAIL_EXIT1 in doit.)

> > +static int
> > +do_test (void)
> > +{
> > +  int ret1 = do_in_chroot_1 (run_chroot_tests);
> > +  if (ret1 == EXIT_UNSUPPORTED)
> > +    return ret1;
> > +
> > +  int ret2 = do_in_chroot_2 (run_chroot_tests);
> 
> Likewise. Use a single "int ret" declaration at the top of the function and
> return directly on error.

Likewise, even if some testcases failed in do_in_chroot_1, I still
want do_in_chroot_2 to run.  Unless ret1==EXIT_UNSUPPORTED (which
happens if we FAIL_UNSUPPORTED), in which case we bail early not
because a testcase failed, but because something indicated that the
test isn't runnable.

-- 
Happy hacking,
~ Luke Shumaker