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

[Ganesh Ajjanagadde]

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.

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