[FFmpeg-devel] Unix "fileno" libavformat module

[William Ahern]

On Tue, Jul 10, 2007 at 05:24:08PM +0200, Benoit Fouet wrote:
<snip>
> > +#include <stdio.h>	/* stderr fputs(3) fprintf(3) */
> >   
> 
> tabs are forbidden

Sorry. I thought I got them all. I've never written code without tabs. Very
confusing. (Don't mean to slight the style.)

> > +#include <errno.h>	/* ENOMEM EINVAL ERANGE */
> > +
> >   
> 
> see AVERROR()

My current branch replaces two of the three uses (AVERROR_NOMEM and
AVERROR_INVALIDDATA), retaining ERANGE as its specified in the standard.

> > +#include <string.h>	/* strncmp(3) */
> > +
> > +#include <sys/param.h>	/* MIN */
> > +
> >   
> 
> see FFMIN

Thanks.

<snip>
> > +typedef struct {
> > +    int fd;
> > +
> > +    struct {
> > +        unsigned char base[65536];
> > +
> >   
> 
> can't this one be dynamically allocated ?

I'd be happy to size the allocation during initialization, much less so
"on-demand". I left it hard-coded because I was unsure of how to employ
ffmpeg facilities for determing such an option, either at compile-time or
run-time. Suggestions?

<snip>
> > +    if (0 == (s = av_malloc(sizeof *s)))
> > +        return -ENOMEM;
> > +
> > +    s->rbuf.end = &s->rbuf.base[sizeof s->rbuf.base];
> > +    s->rbuf.r.pos = s->rbuf.base;
> > +    s->rbuf.w.pos = s->rbuf.base;
> > +
> > +    h->priv_data = s;
> > +
> > +    if (0 != strncmp(uri, "fileno://", sizeof "fileno://" - 1))
> > +        return -EINVAL;
> >   
> 
> mem leak

*blush*

<snip>
> > +        (void)memcpy(buf, s->rbuf.r.pos, size);
> >   
> 
> why this cast ?

Its not an uncommon style to "cast away" a return value for the purposes of
inline documentation. The intent is to say, "I am willfully ignoring the
return value". The sense becomes clearer for things like close(), and,
especially, snprintf(). WIN32 snprintf() returns -1 on overflow, and notably
does not guarantee null-termination for this condition (effectively the
complete opposite of C99 snprintf()). I've noticed this in some of the
ffmpeg code. A (void) cast in such a situation would (or should) serve to
document that the author understood the behavior and implications.

I've incorporated yours and others' critiques into my tree and will create a
new patch.

Gracias.