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

[Ganesh Ajjanagadde]

On Wed, Jan 13, 2016 at 4:05 AM, wm4 <nfxjfg at googlemail.com> wrote:
> On Tue, 12 Jan 2016 10:07:08 -0500
> 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).
>>
>> Again, this is more important for write than read. Or put in other
>
> Indeed, the slides don't mention reading AT ALL.
>
> A quick grep shows that we apparently do at least _something_ with the
> return value of every single fread() call, and most if not all are in
> test/example code.
>
>> words: if the approach of checking at close is going to be nacked no
>> matter what the contents of the message is; I will wash my hands off
>> this issue wrt files opened read-only.
>>
>> >
>> >> Just what is the point?
>> >>
>> >> Can you please stop trolling patches with such gratuitous statements at
>> >> the end?
>
> Isn't my question justified?

Your first point was not:
https://ffmpeg.org/pipermail/ffmpeg-devel/2016-January/186973.html. It
was simply gratuituous flame bait. Keep up the good work of
selectively replying.

> Yes, I might be missing something here,
> but so far I couldn't yet think of a reason why you'd check fclose()
> return values if the file is read-only and if all fread()s are checked
> already.

The freads are checked; but nothing is currently logged during failure modes.

> Wasting my time with reading a PDF that exclusively handling
> _writing_ to _stdout_ (which you do not fclose()) did not help at all.
> Maybe I'm just insane?

Perhaps you are, maybe you aren't. It is irrelevant to the patch. It
was given for justifying the first point, since apparently people like
you did not recognize that this issue affects even the libraries.

>
>> >
>> >
>> > ... Can we not be defensive, please? ...
>>
>> There was nothing "defensive" here. Can you stop dismissing technical
>> comments as "defensive"? This is not the first time you are doing
>> this, and not the first time wm4 has added such useless, provocative
>> statements...
>
> Sorry for pointing out that some of your patches literally do nothing.

They don't do "nothing". See the previous fclose ones, like the one
for ffmpeg.c, or the avfilter ones. No need to feel sorry for
something you are clearly not sorry about, because I or no one else
here cares.

> It's in the nature of the thing: if you flood the ML with many such
> patches (with the threat included of applying them if nobody reviews
> them),

I apply them only when I am convinced universally of their value, and
no one objects to it. Yes, even you wm4, even with all of your
trolling and garbage comments. I take your opinions seriously, whether
or not I agree with them:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/184899.html.
Same goes for the read only business here; I won't apply. As it was
prefaced with "maybe theoretical", even in the absence of review, I
won't apply.

What I am surprised is where these false impressions come from.

> people _might_ be more quick to dismiss them, especially if they
> find strangenesses like this one. Why are you surprised.

I am not surprised. It is well known that you have a habit of trolling
patches here, and I am not the only one who has encountered such
behavior.

>
> This is not so say that these patches are "useless" in general, but if
> they're spiked with definitely useless ones (like the read-only fclose
> so far appear to me to be), then I'll be critical of it.
> (And to be
> honest, I'll just assume that the other supposedly more useful patches
> were done with the same carelessness and equally superficial reasoning.)

They were not done with superficial reasoning. They were clearly
prefixed with statements in the patch notes; I thus acknowledged the
possiblity. It was done for completeness, and so that others with a
lack of knee jerk reactions can comment upon. Assume whatever you feel
like, I don't care:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/182482.html.

>
> Sorry for possibly causing drama, I'll go away now.

Again, simply saying sorry for something that you actually don't care
about. Please do, work on mpv.

> I guess I'll
> concentrate on doing actual work instead,

And you consider mpv "actual work". Not everyone may agree with such
an assessment. I certainly do not. Minds infinitely greater than
yours, mine, or anyone else's here do "actual work". Even on a
relative scale, I don't care about a wastage of human effort on
duplicating media playback, something VLC/mplayer/mplayer2/ffplay and
goodness knows what does.

> who cares if non-sense gets
> pushed to the FFmpeg repo.

I certainly do, and everyone who cares about FFmpeg does. This is
another one of those gratuitous remarks.

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