[FFmpeg-devel] [PATCH] ffmpeg: check fclose return values

[Ganesh Ajjanagadde]

On Jan 8, 2016 1:59 AM, "wm4" <nfxjfg at googlemail.com> wrote:
>
> On Thu, 7 Jan 2016 16:01:14 -0800
> Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>
> > On Thu, Jan 7, 2016 at 3:57 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
wrote:
> > > On Thu, Jan 7, 2016 at 2:18 PM, Michael Niedermayer
> > > <michael at niedermayer.cc> wrote:
> > >> On Thu, Jan 07, 2016 at 11:16:27PM +0100, Michael Niedermayer wrote:
> > >>> On Thu, Jan 07, 2016 at 10:00:47AM -0800, Ganesh Ajjanagadde wrote:
> > >>> > On Thu, Jan 7, 2016 at 9:27 AM, Michael Niedermayer
> > >>> > <michael at niedermayer.cc> wrote:
> > >>> > > On Wed, Jan 06, 2016 at 09:00:46PM -0800, Ganesh Ajjanagadde
wrote:
> > >>> > >> In the spirit of commit a956840cbc. Simple method to reproduce:
> > >>> > >> pass -vstats_file /dev/full to ffmpeg.
> > >>> > >>
> > >>> > >> All raw fclose usages in ffmpeg.c taken care of here.
> > >>> > >>
> > >>> > >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> > >>> > >> ---
> > >>> > >>  ffmpeg.c | 13 ++++++++++---
> > >>> > >>  1 file changed, 10 insertions(+), 3 deletions(-)
> > >>> > >
> > >>> > > LGTM
> > >>> > >
> > >>> > > thanks
> > >>> >
> > >>> > So there is actually a problem with the diagnostic obtained, a
more
> > >>> > accurate diagnostic is via errno, say strerror(errno) instead of
> > >>> > av_err2str(ret).
> > >>> > Comparison:
> > >>> > Error closing vstats file, loss of information possible:
Operation not permitted
> > >>> > vs
> > >>> > Error closing vstats file, loss of information possible: No space
left on device
> > >>> > for the provided /dev/full example.
> > >>> >
> > >>> > So there are a number of possiblities:
> > >>> > 1. Have 2 separate av_log lines, one for each of these.
> > >>> > 2. A single av_log line, using strerror(errno).
> > >>> > 3. Leave as is.
> > >>> >
> > >>> > I prefer 2. Let me know your preference, and I will push later.
> > >>>
> > >>> yes agree, 2.
> > >>
> > >> probably should use av_err2str() instead of strerror() though
> > >
> > > I thought strerror was C89? Your idea unfortunately causes problems
> > > (suspect it is because appropriate error tag is missing):
> > > Error closing vstats file, loss of information possible: Error number
> > > 28 occurred.
> >
> > Did some digging, using strerror should be fine, see 984-989 of
> > cmdutils.c for very similar usage.
>
> strerror() is not thread-safe. There is absolutely no reason for it not
> to be thread-safe, but C is full of idiocy like this.

Thanks for the clarification. Note that ffserver has quite a few usages of
it ;).
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

sent from my phone