Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stefan,

On Tue, 24 Apr 2018, Stefan Beller wrote:

> On Tue, Apr 24, 2018 at 11:51 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> 
> >
> > Oy vey. How many more mistakes can I introduce in one commit...
> >
> 
> I ask this myself all the time, but Software is hard when not having
> computer assisted checks.

Right. I still hope to find some time to play with Infer (Open Source
alternative to Coverity, started at Facebook). Windows support is not
there yet, which is the main hold-up for me (if I had the time, I would
spend it on adding Windows support to Infer). I could imagine that Infer
might be flexible enough to ask these questions programmatically:

- is there a code path that forgets to close() file handles?

- is there a code path that forgets to release strbufs?

- are there redundant strbuf_release() calls?

> The test suite doesn't quite count here, as it doesn't yell loudly
> enough for leaks in corner cases.

Right, and running everything through valgrind is not fast enough.
Besides, this misses non-Linux code paths.

> Thanks for taking these seriously, I was unsure if the
> first issues (close() clobbering the errno) were sever enough
> to bother. It complicates the code, but the effect is theoretical)
> (for EBADF) or a real niche corner case (EINTR).

Maybe it might not be *that* serious currently. It is incorrect, though,
and makes the code less reusable. I like to copy-edit my code a lot when
refactoring is not an option.

> Speaking of that, I wonder if we eventually want to have
> a wrapper
> 
> int xclose(int fd)
> {
>     int err = errno;
>     int ret = close(fd)
>     if (errno == EINTR)
>         /* on linux we don't care about this, other OSes? */
>         ;
>     errno = err;
>     return ret;
> }
> 
> Though not in this series.

Or maybe a Coccinelle rule? (Still not in this series, though.)

Ciao,
Dscho