[FFmpeg-devel] [PATCH] Implement read_file() cmdutils.c option

[Michael Niedermayer]

On Wed, Mar 31, 2010 at 10:06:33PM +0200, Stefano Sabatini wrote:
> On date Tuesday 2010-03-30 15:22:54 +0200, Michael Niedermayer encoded:
> > On Tue, Mar 30, 2010 at 01:38:02AM +0200, Stefano Sabatini wrote:
> > [...]> diff --git a/cmdutils.c b/cmdutils.c
> > > index 1ffe2e8..f561316 100644
> > > --- a/cmdutils.c
> > > +++ b/cmdutils.c
> > > @@ -639,3 +639,26 @@ int read_yesno(void)
> > >  
> > >      return yesno;
> > >  }
> > > +
> > 
> > > +int read_file(const char *filename, char **bufptr, int *size)
> > 
> > int is the wrong type for an array size
> 
> Yes, size_t looks more correct, that's also what is returned by fread(). 
> > 
> > > +{
> > > +    FILE *f = fopen(filename, "r");
> > > +
> > > +    if (!f) {
> > > +        fprintf(stderr, "Cannot read file '%s': %s\n", filename, strerror(errno));
> > > +        return AVERROR(errno);
> > > +    }
> > > +    fseek(f, 0, SEEK_END);
> > 
> > > -                    size = ftell(f);
> > > +    *size = ftell(f) + 1;
> > 
> > you mess +-1 up
> 
> Well like this or either I don't care.
> 
> > [...]
> > > diff --git a/cmdutils.h b/cmdutils.h
> > > index 9190a81..6d12a97 100644
> > > --- a/cmdutils.h
> > > +++ b/cmdutils.h
> > > @@ -200,4 +200,15 @@ void show_pix_fmts(void);
> > >   */
> > >  int read_yesno(void);
> > >  
> > > +/**
> > > + * Reads the file with name filename, and puts its content in a newly
> > > + * allocated 0-terminated buffer.
> > > + *
> > > + * @param bufptr puts here the pointer to the newly allocated buffer
> > > + * @param size puts here the size of the newly allocated buffer
> > > + * @return 0 in case of success, a negative value corresponding to an
> > > + * AVERROR error code in case of failure.
> > 
> > why not return the size?
> 
> size_t is unsigned, returning a negative error code looks nicer and
> easier to integrate.
> 
> Regards.
> -- 
> FFmpeg = Fierce Formidable Maxi Proud Ephemeral Guru

>  cmdutils.c |   23 +++++++++++++++++++++++
>  cmdutils.h |   11 +++++++++++
>  ffmpeg.c   |   21 ++++-----------------
>  3 files changed, 38 insertions(+), 17 deletions(-)
> 484f5e2be5a69bc8082e5c8495974f23427e05a8  0001-Implement-cmdutils.c-read_file-and-use-it-in-ffmpeg..patch
> >From c2d6efefb943ee5789bced3814a168c8790e24ee Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Sun, 28 Mar 2010 16:38:42 +0200
> Subject: [PATCH 1/2] Implement cmdutils.c:read_file(), and use it in ffmpeg.c for reading
>  the second pass encoding log file.
> 
> ---
>  cmdutils.c |   23 +++++++++++++++++++++++
>  cmdutils.h |   11 +++++++++++
>  ffmpeg.c   |   21 ++++-----------------
>  3 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/cmdutils.c b/cmdutils.c
> index c2c44c6..8c5561c 100644
> --- a/cmdutils.c
> +++ b/cmdutils.c
> @@ -639,3 +639,26 @@ int read_yesno(void)
>  
>      return yesno;
>  }
> +
> +int read_file(const char *filename, char **bufptr, size_t *size)
> +{
> +    FILE *f = fopen(filename, "r");
> +
> +    if (!f) {
> +        fprintf(stderr, "Cannot read file '%s': %s\n", filename, strerror(errno));
> +        return AVERROR(errno);
> +    }
> +    fseek(f, 0, SEEK_END);
> +    *size = ftell(f);
> +    fseek(f, 0, SEEK_SET);
> +    *bufptr = av_malloc(*size + 1);
> +    if (!*bufptr) {
> +        fprintf(stderr, "Could not allocate file buffer\n");
> +        return AVERROR(ENOMEM);
> +    }

missing fclose()
[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/20100331/552c2cdf/attachment.pgp>