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

[Christian Brauner <christian dot brauner at mailbox dot org>]

On Sun, Nov 12, 2017 at 10:49:19AM -0500, Luke Shumaker wrote:
> 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.

Since these a) are tests and b) if variable declarations not at the beginning of
a new scope but in the middle of the current scope are considered acceptable who
am I to yell. :)

Thanks!
Christian

> 
> > > +  /* 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

Attachment: signature.asc
Description: PGP signature