[FFmpeg-devel] [PATCH 00/13] check all fclose usage
gajjanag at mit.edu
Tue Jan 12 16:56:05 CET 2016
On Tue, Jan 12, 2016 at 10:29 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> 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, therefore
> the user (logically) thinks: "FFmpeg's decoder sucks. FFmpeg sucks." That's
> 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.
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
More information about the ffmpeg-devel