On Wed, Jan 13, 2016 at 10:48 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Wed, Jan 13, 2016 at 4:05 AM, wm4 <nfx...@googlemail.com> wrote: >> On Tue, 12 Jan 2016 10:07:08 -0500 >> Ganesh Ajjanagadde <gajja...@mit.edu> 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
