[FFmpeg-devel] [PATCH] Make cmdutils.c:print_error() use strerror() if av_strerror() fails (e.g. if strerror_r() is not defined)

[Michael Niedermayer]

On Tue, May 04, 2010 at 03:41:46PM +0200, Stefano Sabatini wrote:
> On date Tuesday 2010-05-04 04:53:39 +0200, Michael Niedermayer encoded:
> > On Tue, May 04, 2010 at 12:03:33AM +0200, Stefano Sabatini wrote:
> > > Hi, subject says it all.
> > > 
> > > This is meant to fix issue #1894.
> > > 
> > > Regards.
> > > -- 
> > > FFmpeg = Fiendish and Fundamentalist Mastodontic Philosophical Elitist Genius
> > 
> > >  error.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 218a948be301740f83d4e120453ae4a4c68728f2  0001-Make-av_strerror-return-1-even-in-the-case-when-av_s.patch
> > > >From e1eeee18829336838ad5c05b9480d798bf3aeb96 Mon Sep 17 00:00:00 2001
> > > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > > Date: Mon, 3 May 2010 23:28:01 +0200
> > > Subject: [PATCH 1/3] Make av_strerror() return -1 even in the case when av_strerror_r() is
> > >  not defined.
> > > 
> > > ---
> > >  libavutil/error.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/libavutil/error.c b/libavutil/error.c
> > > index 3dd38a3..cb9a73c 100644
> > > --- a/libavutil/error.c
> > > +++ b/libavutil/error.c
> > > @@ -34,10 +34,11 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
> > >      if (errstr) {
> > >          av_strlcpy(errbuf, errstr, errbuf_size);
> > >      } else {
> > > +        ret = -1;
> > >  #if HAVE_STRERROR_R
> > >          ret = strerror_r(AVUNERROR(errnum), errbuf, errbuf_size);
> > >  #endif
> > > -        if (!HAVE_STRERROR_R || ret < 0)
> > > +        if (ret < 0)
> > >              snprintf(errbuf, errbuf_size, "Error number %d occurred", errnum);
> > >      }
> > 
> > you mess up the return code
> 
> The idea is to issue -1 also in the case where HAVE_STRERROR_R is not
> defined and that the error code is not recognized, this way the
> calling function knows that a code description was not given.
> 
> Check if you prefer the new patch approach.
> 
> > 
> > >  
> > > -- 
> > > 1.7.0
> > > 
> > 
> > >  cmdutils.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > cb66fd1350d7bb6698b1ed175f66dcb8f7f4334f  0002-Make-print_error-use-strerror-in-case-av_strerror-fa.patch
> > > >From 9050e3ac4a1c2aab6c413a71b484a4fa89281c3a Mon Sep 17 00:00:00 2001
> > > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > > Date: Mon, 3 May 2010 23:36:21 +0200
> > > Subject: [PATCH 2/3] Make print_error() use strerror() in case av_strerror() fails.
> > > 
> > > Should provide a meaningful error message for systems which do not
> > > support strerror_r().
> > > 
> > > Fix roundup issue #1894.
> > > ---
> > >  cmdutils.c |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/cmdutils.c b/cmdutils.c
> > > index e6efc49..0de1d2d 100644
> > > --- a/cmdutils.c
> > > +++ b/cmdutils.c
> > > @@ -300,8 +300,10 @@ void print_error(const char *filename, int err)
> > >          break;
> > >  #endif
> > >      default:
> > > -        av_strerror(err, errbuf, sizeof(errbuf));
> > > -        fprintf(stderr, "%s: %s\n", filename, errbuf);
> > > +        if (av_strerror(err, errbuf, sizeof(errbuf)) < 0)
> > > +            fprintf(stderr, "%s: %s\n", filename, strerror(AVUNERROR(err)));
> > > +        else
> > > +            fprintf(stderr, "%s: %s\n", filename, errbuf);
> > 
> > missing {} and you should document the thread saftey issues tis introduces
> > users of the code likely want to know
> 
> Uhm it's a cmdutils.c function, I don't think we need to document
> this. Note also that the usage in ffplay of print_error() looks safe
> anyway, since print_error() is used only in the init phase, with just
> one active thread.

you need to document it if you want me to approve it ;)


> 
> > besides
> > errbuf= strerror(AVUNERROR(err)
> > should simplify this
> 
> Regards.
> -- 
> FFmpeg = Fierce and Fantastic Maxi Powerful Exuberant Governor

>  error.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 5193f91d79d16d92f8585dcc45f02296c2179204  0001-Make-av_strerror-return-1-even-in-the-case-when-av_s.patch
> >From 60839e54dcfe22769778e00bf8a0f696bc073ff5 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Mon, 3 May 2010 23:28:01 +0200
> Subject: [PATCH 1/3] Make av_strerror() return -1 even in the case when av_strerror_r() is
>  not defined.
> 
> This allows applications to check if av_strerror() cannot provide a
> meaningful representation for the provided error code, without to
> actually check the filled string.
> ---
>  libavutil/error.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/libavutil/error.c b/libavutil/error.c
> index 3dd38a3..b6d6019 100644
> --- a/libavutil/error.c
> +++ b/libavutil/error.c
> @@ -36,8 +36,10 @@ int av_strerror(int errnum, char *errbuf, size_t errbuf_size)
>      } else {
>  #if HAVE_STRERROR_R
>          ret = strerror_r(AVUNERROR(errnum), errbuf, errbuf_size);
> +#else
> +        ret = -1;
>  #endif
> -        if (!HAVE_STRERROR_R || ret < 0)
> +        if (ret < 0)
>              snprintf(errbuf, errbuf_size, "Error number %d occurred", errnum);
>      }

you need to document the -1 case as well

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100504/f699c3df/attachment.pgp>