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

[Ganesh Ajjanagadde]

On Tue, Jan 12, 2016 at 1:18 PM, Paul B Mahol <onemda at gmail.com> wrote:
> On 1/12/16, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>> On Tue, Jan 12, 2016 at 10:29 AM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>>> Hi,
>>>
>>> On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> wrote:
>>>
>>>> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje <rsbultje at gmail.com>
>>>> wrote:
>>>> > Hi,
>>>> >
>>>> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>>> > wrote:
>>>> >
>>>> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4 <nfxjfg at googlemail.com> wrote:
>>>> >> > This makes no sense. Even if fclose() should fail for
>>>> >> > whatever obscure reasons there might be, reading already worked
>>>> >> > without errors, and thus closing failure has no meaning to use.
>>>> >>
>>>> >> Well, reading may not have worked, and the fclose may have been called
>>>> >> in a failure path.
>>>> >
>>>> >
>>>> > Then the error should be in the code path of fread(), not fclose().
>>>> > Displaying an error (in whatever way) related to close while the actual
>>>> > problem was reading data is going to be confusing to the user.
>>>>
>>>> Read the rest of it; checking for every fread/fseek is not productive;
>>>> there are at least 3 of fread/fseek in the example I gave. Printing at
>>>> the time of closure is a natural means of doing things, again see:
>>>> https://www.gnu.org/ghm/2011/paris/slides/jim-meyering-goodbye-world.pdf
>>>> (particularly slide 7).
>>>
>>>
>>> He's very smart, but you still have to see his advice in context, also see
>>> [1].
>>
>> The question of his smartness is irrelevant, as is the appeal to
>> authority definition here.
>>
>>>
>>> Most production uses of ffmpeg involve long-running processes in a
>>> multi-component pipeline, where fail-to-read will cause errors downstream.
>>> Reading the file from disk is only one small component. Let's say that I
>>> read a buffer of 10 bytes but I only got 5 because whatever went wrong. I
>>> parse the input buffer of 5 bytes, it's obviously corrupt, decoding stream
>>> goes wrong, and the decoder displays error ("ffvp9: corrupt input stream,
>>> failed to decode"). Note how this is the topmost error in stderr,
>>> therefore
>>> the user (logically) thinks: "FFmpeg's decoder sucks. FFmpeg sucks."
>>> That's
>>> bad. [2]
>>
>> If the fread is unchecked, that is a separate issue. At least for the
>> example I gave, the fread is checked. However, there is little (if
>> anything) that gets logged, since only the return value is set. The
>> question is where/what to log, if any.
>>
>> Instead of making such generic statements, please post comments to the
>> actual patches; the discussion will be more productive.
>>
>>>
>>> The first error should (ideally) be indicative of the actual problem. So,
>>> if the read is the error, the error should be associated with that read
>>> early on to help the user diagnose the actual source of the problem.
>>
>> Well, how about littering the code all over the place for every
>> read/seek/puts, etc so that your "ideal" can be met.
>>
>>>
>>> Ronald
>>>
>>> [1] https://en.wikipedia.org/wiki/Argument_from_authority
>>> [2] 10s or 1000s of bytes or frames failing to decode later, there's some
>>> little error saying the file descriptor failed to close. Did you ever look
>>> at line 1000 in your error log?
>>
>> There is something called grep. And ironically your proposed idea of
>> logging at the fread does not help with this either.
>
> Why the stuff I post have no comments at all?
> But this marginal functionality is commented so much.

It takes almost no effort to write endlesslessly on simple things.
More complex patches take greater effort on the part of the reviewer,
and will draw fewer replies.

It also is in the nature of the work I have taken up, and the quite
different perspective I have from many others here. This draws noise.
I try not to react to the useless flame bait, but unfortunately fail
at that sometimes. As a zeroth order approximation, one can say that I
am a technical purist and many others here are pragmatists. Nothing on
ffmpeg-devel or IRC so far has convinced me of the superiority of the
"pragmatist" approach. If anything, things like
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/186109.html,
https://ffmpeg.org/pipermail/ffmpeg-devel/2016-January/186363.html vs
https://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2015-December/003293.html
(first comment from atomnuker) make me value technical purity.

I wish I could simply let go of these things and work on actual stuff.
The problem is that I have too many silly things like this on my todo
list that are extremely low hanging, and trouble me when I see them. I
don't like working on them at all, and still less dealing with useless
comments. The problem is that very few work on it and it troubles me,
so I take it upon myself.

What astonishes me is that when essentially every C/systems
programming resource asks people to check return values of system
calls/library functions, it is still not done in many cases in FFmpeg.
This has occurred multiple times; even now we have some unchecked
malloc's.

I just hope that over time, I will find less of these silly things. I
am not too optimistic: every time I spend 5 minutes looking at the
codebase, I find something trivial like this.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel