[FFmpeg-devel] [PATCH] lavu: add av_bprintf and related.

[Stefano Sabatini]

On date Sunday 2012-03-11 17:20:38 +0100, Nicolas George encoded:
> 
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  doc/APIchanges     |    3 +
>  libavutil/Makefile |    6 +-
>  libavutil/avutil.h |    2 +-
>  libavutil/bprint.c |  198 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/bprint.h |  132 ++++++++++++++++++++++++++++++++++
>  5 files changed, 338 insertions(+), 3 deletions(-)
>  create mode 100644 libavutil/bprint.c
>  create mode 100644 libavutil/bprint.h
[...]
> +void av_bprintf(AVBPrint *buf, const char *fmt, ...)
> +{
> +    unsigned room;
> +    char *dst;
> +    va_list vl;

> +    int add;

nit+++: maybe len_add to stress the fact that it represents a length

> +
> +    while (1) {
> +        room = av_bprint_room(buf);
> +        dst = room ? buf->str + buf->len : NULL;
> +        va_start(vl, fmt);
> +        add = vsnprintf(dst, room, fmt, vl);
> +        va_end(vl);
> +        if (add <= 0)
> +            return;
> +        if (add < room)
> +            break;
> +        if (av_bprint_grow(buf, 0))
> +            break;
> +    }

> +    /* arbitrary margin to avoid small overflows */
> +    add = FFMIN(add, UINT_MAX - 5 - buf->len);

I'm not sure I understand the need for this check.

room provides the available size left in the buffer, since the max
size is UINT_MAX then we're sure that:
len+room <= UINT_MAX

and so since add+1 <= room then:
len+add+1 <= UINT_MAX.

So len+add can't be greater than UINT_MAX-1. What's the gain if you
restrict the maximum recorded len to UINT_MAX - 5? Especially
considering that the real len will be len+add.

> +    buf->len += add;
> +}
> +
> +void av_bprint_chars(AVBPrint *buf, char c, unsigned n)
> +{
> +    unsigned room, real_n;
> +
> +    while (1) {
> +        room = av_bprint_room(buf);
> +        if (n < room)
> +            break;
> +        if (av_bprint_grow(buf, 0))
> +            break;
> +    }
> +    if (room) {
> +        real_n = FFMIN(n, room - 1);
> +        memset(buf->str + buf->len, c, real_n);
> +        buf->str[buf->len + real_n] = 0;
> +    }

> +    buf->len += FFMIN(n, UINT_MAX - 5 - buf->len);

ditto

[...]

Looks fine otherwise.
-- 
FFmpeg = Faithful & Fundamental Mournful Peaceful Ecletic Guru