Re: [PATCH v2 5/5] linux ttyname and ttyname_r: Add tests [BZ #22145]

[Luke Shumaker <lukeshu at lukeshu dot com>]

On Wed, 08 Nov 2017 18:23:49 -0500,
Luke Shumaker wrote:
> > > +	    FAIL_EXIT1 ("write (uid_map, \"%s\"): %m", str);
> > 
> > I haven't looked at the test code a lot yet but is it generally considered ok to
> > not cleanup before exit? That's mostly style but still it'd be good to clarify
> > this. I usually do all the cleanup even when exiting.
> 
> Clean up the file system, or clean up memory?  FS cleanup isn't
> necessary because we do everything on a tmpfs in a new mount
> namespace.  Looking at other tests, it seems to be OK to leave memory
> messy when exiting with an error.

(if it weren't OK to leave the memory a mess, then the x* functions in
support/ wouldn't be calling FAIL_EXIT1 behind your back)

> > > +    ok = doit (slave, "with readlink trap; no fallback",
> > > +               (struct result_r){.name=NULL, .ret=ENODEV, .err=ENODEV}) && ok;
> > 
> > Same comment as above: Imho, this is not very nice. It looks clever but is hard
> > to parse semantically. I'd prefer if the assignment would be entangled from the
> > following "&& ret" check. This is just easier to maintain.
> 
> How would you rather it be written?
> 
>     static void
>     doit (..., bool *ok)
>     {
>       ...
> 
>       if (!eq_ttyname (run_ttyname (fd), expected))
>         *ok = false;
>       if (!eq_ttyname_r (run_ttyname_r (fd), expected_r))
>         *ok = false;
>     }
>   
>     doit(..., &ok);
> 
> ?

Disregard that, I was being silly,

    if (!doit (...))
      ok = false;

doesn't add too much noise.

-- 
Happy hacking,
~ Luke Shumaker