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

[wm4]

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.