Re: [PATCH v2] config: avoid "write_in_full(fd, buf, len) < len" pattern

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 13, 2017, 19:18 UTC
Message-ID: <20170913191840.GG27425@aiede.mtv.corp.google.com>
In-Reply-To: <20170913185848.5ht44spbakzkrsu6@sigill.intra.peff.net>

Jeff King wrote:

> On Wed, Sep 13, 2017 at 11:24:31AM -0700, Jonathan Nieder wrote:

Show 15 quoted lines

>> Compilers' signed/unsigned comparison warning can be noisy, but I'm
>> starting to feel it's worth the suppression noise to turn it on when
>> DEVELOPER=1 anyway.  What do you think?  Is there a way to turn it on
>> selectively for certain functions on the LHS (like read() and write()
>> style functions)?
>
> Obviously we couldn't turn them on for -Werror at this point. Let me see
> how bad "-Wsign-compare -Wno-error=sign-compare" is.
>
>   $ make 2>&1 | grep -c warning:
>   1391
>
> Ouch. I'm afraid that's probably not going to be very helpful. Even if I
> were introducing a new problem, I'm not likely to see it amidst the mass
> of existing complaints.

Yup.

Something like http://coccinelle.lip6.fr/rules/find_unsigned.html (but not that one precisely, since it's not smart enough to follow typedefs like size_t!) can do this for specific functions. Then "make coccicheck" would report new problematic usages. Let me know if this seems worth pursuing, and I'll send a patch based on the spatch I used to review your patch 8.

Using coccinelle for this is a kind of oversized hammer. I wonder if e.g. sparse would be able to do a better job.

> AFAIK there's no way to turn it on for specific functions, but I'm far
> from a gcc-warning guru. Even if you could, though, the error may be far
> from the function (e.g., if we store the result in an ssize_t and then
> compare that with a size_t).

If we have a check that catches the obvious cases then I'm already pretty happy.

Though checking ssize_t vs size_t comparisons in general might also not be a bad idea, since it would be narrower than the general -Wsign-compare check.

Thanks, Jonathan