[FFmpeg-devel] [PATCH 00/13] check all fclose usage
Paul B Mahol
onemda at gmail.com
Tue Jan 12 19:18:35 CET 2016
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>
>> On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje <rsbultje at gmail.com>
>>> > 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:
>>> (particularly slide 7).
>> He's very smart, but you still have to see his advice in context, also see
> 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,
>> the user (logically) thinks: "FFmpeg's decoder sucks. FFmpeg sucks."
>> bad. 
> 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.
>>  https://en.wikipedia.org/wiki/Argument_from_authority
>>  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.
More information about the ffmpeg-devel