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

[Ganesh Ajjanagadde]

On Wed, Jan 13, 2016 at 10:48 AM, Ganesh Ajjanagadde  wrote:
> On Wed, Jan 13, 2016 at 4:05 AM, wm4  wrote:
>> On Tue, 12 Jan 2016 10:07:08 -0500
>> Ganesh Ajjanagadde  wrote:
[...]
>
>
> 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.

Even in the read only case, there turns out to be value here. For
instance, currently reads are mostly checked via an if (fread(...));
etc. Nothing wrong, it is a clean, maintainable way of doing things.
The "proper" thing to do is for every fread, check feof vs ferror, and
I think close immediately at the point of ferror, with a diagnostic
saying that reads failed, or something like that. The problem is that,
as easily seen and also expressed by the slides, this is an
unmaintainable solution. Maybe it is ok here, since there are < 15
files in the project with raw freads. Michael is ok with it for
ffmpeg_opt, something he maintains. It remains to be seen what wm4 and
others think about it. But in general, no, as seen with the next
example.

For example, look at avio_r8, avio_r16, etc. They are deliberately
unchecked. It will cause useless loss of speed/horrible verbosity if
they were checked. On the other hand, by checking avio_close, at least
something is printed when things go wrong, with minimal verbosity.

By checking at the point of closure, one can at least inform that
there was some i/o failure. The current patchset is a "sloppier"
approximation to the more complete gnulib solution (expressed in the
slides), since it does not handle the case when the reads fail but
close succeeds, and similar corner cases. An example of this in the
slides is when buffering is disabled, invoked there via an stdbuf
--output=0. I can't think of a way to make an equivalent happen for
reads in non-cli tools in FFmpeg; hence I deemed the sloppiness
acceptable for the moment.

I also anticipated some negative reactions (though not their
magnitude) from some here, and favored a smaller initial step. I
change my mind regarding proceeding/not proceeding with this, since I
am now convinced even in the read-only case. As for whether the
patches are fine in their current form, I really don't know. I chose
them as a compromise between "proper" behavior and no diagnostics. In
all instances, I did not think they were important enough to warrant
forwarding an error return code, but again I don't know.

I think the best way to achieve common understanding here is via IRC.

[...]
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Tue, 12 Jan 2016 10:07:08 -0500
Ganesh Ajjanagadde  wrote:

> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje  wrote:
> > Hi,
> >
> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
> > wrote:
> >  
> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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? 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. 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?

> >
> >
> > ... 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.
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), people _might_ be more quick to dismiss them, especially if they
find strangenesses like this one. Why are you surprised.

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.)

Sorry for possibly causing drama, I'll go away now. I guess I'll
concentrate on doing actual work instead, who cares if non-sense gets
pushed to the FFmpeg repo.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Wed, 13 Jan 2016 10:48:01 -0500
Ganesh Ajjanagadde  wrote:

> On Wed, Jan 13, 2016 at 4:05 AM, wm4  wrote:
> > On Tue, 12 Jan 2016 10:07:08 -0500
> > Ganesh Ajjanagadde  wrote:
> >  
> >> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje  
> >> wrote:  
> >> > Hi,
> >> >
> >> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
> >> > wrote:
> >> >  
> >> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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.

Both of these are perfectly valid points. To express the first in a
completely different way: should scripting/API users be forced to
parse FFmpeg log message to catch failures? Depending on the answer,
your patches are probably good enough, so don't mind me.

> > 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.

Oh no, I'm using FFmepg for something! I should be fucking ashamed. My
perspective might be somewhat locked-in to the software that I'm
maintaining and that happens to rely a lot on FFmpeg, but I don't think
it's as bad as you're trying to put all the time. Also I never implied
"mpv" when I wrote "actual work" above. Why are you focusing so much on
it?

Plus, I'm not the only one who works on software that uses FFmpeg.
There are many developers who use FFmpeg or its libraries selfishly for
their very own projects, and whose contributions or comments are
sometimes very specific to their own needs, yet they never get accused
of it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Wed, Jan 13, 2016 at 1:14 PM, wm4  wrote:
> On Wed, 13 Jan 2016 10:48:01 -0500
> Ganesh Ajjanagadde  wrote:
>
>> On Wed, Jan 13, 2016 at 4:05 AM, wm4  wrote:
>> > On Tue, 12 Jan 2016 10:07:08 -0500
>> > Ganesh Ajjanagadde  wrote:
>> >
>> >> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje  
>> >> wrote:
>> >> > Hi,
>> >> >
>> >> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
>> >> > wrote:
>> >> >
>> >> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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.
>
> Both of these are perfectly valid points. To express the first in a
> completely different way: should scripting/API users be forced to
> parse FFmpeg log message to catch failures? Depending on the answer,
> your patches are probably good enough, so don't mind me.

You are spinning the point I made.

>
>> > 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.
>
> Oh no, I'm using FFmepg for something! I should be fucking ashamed. My
> perspective might be somewhat locked-in to the software that I'm
> maintaining and that happens to rely a lot on FFmpeg, but I don't think
> it's as bad as you're trying to put all the time.

> Also I never implied
> "mpv" when I wrote "actual work" above. Why are you focusing so much on
> it?

I gave it as an illustration, since that is where the bulk of your
activity is per github.

>
> Plus, I'm not the only one who works on software that uses FFmpeg.
> There are many developers who use FFmpeg or its libraries selfishly for
> their very own projects, and whose contributions or comments are
> sometimes very specific to their own needs, yet they never get accused
> of it.

Maybe. Does not matter to the question right here what other users do.
As this is mostly off-topic, please come to IRC, I am there right now.
I can answer all questions (this and/or anything else that annoys
you).

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Wed, Jan 13, 2016 at 4:05 AM, wm4  wrote:
> On Tue, 12 Jan 2016 10:07:08 -0500
> Ganesh Ajjanagadde  wrote:
>
>> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje  wrote:
>> > Hi,
>> >
>> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
>> > wrote:
>> >
>> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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:

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

[Reynaldo H. Verdejo Pinochet]

Hi Ganesh

Somehow I'm missing your ffserver patches on this thread, had
to check them on gmame. Probably something odd with my local
filters. Commenting offline for the time being:

09/13 Its OK but actually introduces the error (-  }) you fix on
10/13... clean up accordingly
11, 12 & 13 LGTM


Bests,

-- 
Reynaldo H. Verdejo Pinochet
Open Source Group
Samsung Research America / Silicon Valley
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Mon, 11 Jan 2016 23:25:02 -0500
Ganesh Ajjanagadde  wrote:

> Some preliminary work has already been done on fclose checking. This completes
> the work, modulo a few exceptions:

1. Printing warnings is completely useless unless maybe in ffmpeg.c
interactive usage (if the user has good eyes). For API users or
ffmpeg.c automated shell scripts this helps absolutely nothing.
2. There are some "theoretical" checks for fclose() on files opened in
read-only mode. 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.

Just what is the point?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Tue, Jan 12, 2016 at 1:18 PM, Paul B Mahol  wrote:
> On 1/12/16, Ganesh Ajjanagadde  wrote:
>> On Tue, Jan 12, 2016 at 10:29 AM, Ronald S. Bultje 
>> wrote:
>>> Hi,
>>>
>>> On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde 
>>> wrote:
>>>
 On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje 
 wrote:
 > Hi,
 >
 > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
 > wrote:
 >
 >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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.

It takes almost no effort to write endlesslessly on simple things.
More complex patches take greater effort on the part of the reviewer,
and will draw fewer replies.

It also is in the nature of the work I have taken up, and the quite
different perspective I have from many others here. This draws noise.
I try not to react to the useless flame bait, but unfortunately fail
at that sometimes. As a zeroth order approximation, one can say that I
am a technical purist and many others here are pragmatists. Nothing on
ffmpeg-devel or IRC so far has convinced me of the superiority of the
"pragmatist" approach. If anything, things like
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/186109.html,
https://ffmpeg.org/pipermail/ffmpeg-devel/2016-January/186363.html vs
https://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2015-December/003293.html
(first comment from atomnuker) make me value technical purity.

I wish I could simply let go of these things and work on actual stuff.
The problem is that I have too many silly things like this on my todo
list that are extremely low hanging, and trouble me when I see them. I
don't like working on them at all, and still less dealing with useless
comments. The problem is that very few work on it and it troubles me,
so I take it upon myself.

What astonishes me is that when essentially every C/systems
programming resource asks 

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

[Ronald S. Bultje]

Hi,

On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
wrote:

> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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.

> Just what is the point?
>
> Can you please stop trolling patches with such gratuitous statements at
> the end?


... Can we not be defensive, please? ...

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje  wrote:
> Hi,
>
> On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
> wrote:
>
>> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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
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?
>
>
> ... 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...

>
> Ronald
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde 
wrote:

> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje 
> wrote:
> > Hi,
> >
> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
> > wrote:
> >
> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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].

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]

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.

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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Tue, Jan 12, 2016 at 10:29 AM, Ronald S. Bultje  wrote:
> Hi,
>
> On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde 
> wrote:
>
>> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje 
>> wrote:
>> > Hi,
>> >
>> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
>> > wrote:
>> >
>> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Paul B Mahol]

On 1/12/16, Ganesh Ajjanagadde  wrote:
> On Tue, Jan 12, 2016 at 10:29 AM, Ronald S. Bultje 
> wrote:
>> Hi,
>>
>> On Tue, Jan 12, 2016 at 10:07 AM, Ganesh Ajjanagadde 
>> wrote:
>>
>>> On Tue, Jan 12, 2016 at 9:43 AM, Ronald S. Bultje 
>>> wrote:
>>> > Hi,
>>> >
>>> > On Tue, Jan 12, 2016 at 7:52 AM, Ganesh Ajjanagadde 
>>> > wrote:
>>> >
>>> >> On Tue, Jan 12, 2016 at 4:38 AM, wm4  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

Some preliminary work has already been done on fclose checking. This completes
the work, modulo a few exceptions:
1. Things under an ifdef DEBUG or similar: these are not important.
2. Tests/tools code: also not important.
3. Likely not important, and beyond my knowledge: see lavu/arm/cpu.c. fclose is
done on read-only files, see get_cpuinfo, get_hwcap. Thus,
the importance of checking the return value is relatively little. Furthermore, 
/proc/cpuinfo is a method
that only works on Linux, on BSD's sysctl or other method must be used:
https://lists.freebsd.org/pipermail/freebsd-questions/2006-March/117134.html.
Thus, a proper, portable solution may bypass fclose altogether.

Note that the choice of error level, e.g WARNING vs ERROR is subjective. 
Generally,
for the read only case, WARNING is used. In no case is the error propagated out
of the function; cursory glances reveal that it is usually not a critical error.
I may be entirely wrong in some cases.

In the case of ffserver, some nearby improvements were done. Most serious was 
the
build failure on non Linux machines.

Ganesh Ajjanagadde (13):
  lavc/dvdsubdec: check fclose return value
  lavfi/vf_deshake: check fclose return value
  lavfi/vf_lut3d: check fclose return value
  lavfi/vf_paletteuse: check fclose return value
  lavfi/vf_psnr: check fclose return value
  lavfi/vf_ssim: check fclose return value
  lavfi/vf_vidstabdetect: check fclose return value
  lavfi/vf_vidstabtransform: check fclose return value
  ffserver: check fclose return value
  ffserver: fix build failure on non linux machines
  ffserver: log diagnostics for popen return failure
  ffserver: correct indentation
  ffserver_config: check fclose return value

 ffserver.c| 48 +++
 ffserver_config.c | 10 ++--
 libavcodec/dvdsubdec.c|  3 ++-
 libavfilter/vf_deshake.c  |  5 +++-
 libavfilter/vf_lut3d.c|  3 ++-
 libavfilter/vf_paletteuse.c   |  8 ++-
 libavfilter/vf_psnr.c |  5 +++-
 libavfilter/vf_ssim.c |  5 +++-
 libavfilter/vf_vidstabdetect.c|  5 +++-
 libavfilter/vf_vidstabtransform.c |  4 +++-
 10 files changed, 72 insertions(+), 24 deletions(-)

-- 
2.7.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

On Mon, Jan 11, 2016 at 11:25 PM, Ganesh Ajjanagadde
 wrote:
[...]
>
> In the case of ffserver, some nearby improvements were done. Most serious was 
> the
> build failure on non Linux machines.

This was a completely bogus one; an error made while
rebasing/squashing stuff. Sorry for the noise. Amended locally (squash
the first 2 ffserver patches together, i.e 9 and 10).
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel