[FFmpeg-devel] [PATCH 00/13] check all fclose usage

[Ganesh Ajjanagadde]

On Wed, Jan 13, 2016 at 10:48 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Wed, Jan 13, 2016 at 4:05 AM, wm4 <nfxjfg at googlemail.com> wrote:
>> On Tue, 12 Jan 2016 10:07:08 -0500
>> Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
[...]
>
>
> I apply them only when I am convinced universally of their value, and
> no one objects to it. Yes, even you wm4, even with all of your
> trolling and garbage comments. I take your opinions seriously, whether
> or not I agree with them:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/184899.html.
> Same goes for the read only business here; I won't apply. As it was
> prefaced with "maybe theoretical", even in the absence of review, I
> won't apply.

Even in the read only case, there turns out to be value here. For
instance, currently reads are mostly checked via an if (fread(...));
etc. Nothing wrong, it is a clean, maintainable way of doing things.
The "proper" thing to do is for every fread, check feof vs ferror, and
I think close immediately at the point of ferror, with a diagnostic
saying that reads failed, or something like that. The problem is that,
as easily seen and also expressed by the slides, this is an
unmaintainable solution. Maybe it is ok here, since there are < 15
files in the project with raw freads. Michael is ok with it for
ffmpeg_opt, something he maintains. It remains to be seen what wm4 and
others think about it. But in general, no, as seen with the next
example.

For example, look at avio_r8, avio_r16, etc. They are deliberately
unchecked. It will cause useless loss of speed/horrible verbosity if
they were checked. On the other hand, by checking avio_close, at least
something is printed when things go wrong, with minimal verbosity.

By checking at the point of closure, one can at least inform that
there was some i/o failure. The current patchset is a "sloppier"
approximation to the more complete gnulib solution (expressed in the
slides), since it does not handle the case when the reads fail but
close succeeds, and similar corner cases. An example of this in the
slides is when buffering is disabled, invoked there via an stdbuf
--output=0. I can't think of a way to make an equivalent happen for
reads in non-cli tools in FFmpeg; hence I deemed the sloppiness
acceptable for the moment.

I also anticipated some negative reactions (though not their
magnitude) from some here, and favored a smaller initial step. I
change my mind regarding proceeding/not proceeding with this, since I
am now convinced even in the read-only case. As for whether the
patches are fine in their current form, I really don't know. I chose
them as a compromise between "proper" behavior and no diagnostics. In
all instances, I did not think they were important enough to warrant
forwarding an error return code, but again I don't know.

I think the best way to achieve common understanding here is via IRC.

[...]