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

[Paul B Mahol]

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.