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

[Ganesh Ajjanagadde]

On Thu, Jan 7, 2016 at 4:34 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Thu, Jan 07, 2016 at 04:01:14PM -0800, Ganesh Ajjanagadde 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.
>
> probably but why does av_err2str not work ? did you use
> AVERROR(errno) ?

Oops, passed it directly without going through AVERROR. Fixed, pushed.
Thanks for your patience and review.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>