[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 Wed, May 05, 2010 at 12:37:52AM +0200, Stefano Sabatini wrote:
> On date Tuesday 2010-05-04 17:12:24 +0200, Michael Niedermayer encoded:
> > On Tue, May 04, 2010 at 03:41:46PM +0200, Stefano Sabatini wrote:
> [...]
> > > > > 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 ;)
> 
> Revisited patch and documentation for print_error() attached.
> 
> Regards.
> -- 
> FFmpeg = Fiendish and Friendly Multimedia Purposeless Ecumenical God

>  cmdutils.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 8e674f8a79111f0d7264d7827a430278287a03fb  0002-Make-print_error-use-strerror-in-case-av_strerror-fa.patch
> >From a079441a9dbdca615cac8badf376ad9c97e4d3eb 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/6] 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(-)

ok
[...]
>  cmdutils.h |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 386c4acd4be5a4e8b73567c40fb89a3164d67089  0003-Document-cmdutils.c-print_error.patch
> >From f41624ff126f9ee41a2ac9dbaef6c49d0a103a1d Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Wed, 5 May 2010 00:11:57 +0200
> Subject: [PATCH 3/6] Document cmdutils.c:print_error().
> 
> ---
>  cmdutils.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/cmdutils.h b/cmdutils.h
> index ad8cefd..86da06c 100644
> --- a/cmdutils.h
> +++ b/cmdutils.h
> @@ -134,6 +134,14 @@ void parse_options(int argc, char **argv, const OptionDef *options,
>  
>  void set_context_opts(void *ctx, void *opts_ctx, int flags);
>  
> +/**
> + * Prints a string indicating filename and a description of the error
> + * code err to stderr.

this sounds a bit uneasy and rough

what about
Prints a human readable error message



> + * The use of this function may be unsafe in a multithreaded
> + * application.

... if have_strerwhatere_r is not available

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

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- 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/20100505/a1bfd583/attachment.pgp>