Re: [FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Marton Balint <cus@passwd.hu>]

On Mon, 3 Jan 2022, Andreas Rheinhardt wrote:

> Marton Balint:
>> 
>> 
>> On Fri, 31 Dec 2021, Marton Balint wrote:
>> 
>>>
>>>
>>> On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:
>>>
>>>>  Marton Balint:
>>>>>  This makes sure the error condition is kept in AVIOContext even if the
>>>>>  user
>>>>>  does not check the return value of avio_read_to_bprint or
>>>>>  ff_read_line_to_bprint.
>>>>>
>>>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>>  ---
>>>>>   libavformat/aviobuf.c | 8 ++++++--
>>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>>  diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>>>>  index 29d4bd7510..6f8a822ee3 100644
>>>>>  --- a/libavformat/aviobuf.c
>>>>>  +++ b/libavformat/aviobuf.c
>>>>>  @@ -875,8 +875,10 @@ static int64_t
>>>>>  read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>>>>       if (ret < 0)
>>>>>           return ret;
>>>>>
>>>>>  -    if (!av_bprint_is_complete(bp))
>>>>>  +    if (!av_bprint_is_complete(bp)) {
>>>>>  +        s->error = AVERROR(ENOMEM);
>>>>>           return AVERROR(ENOMEM);
>>>>>  +    }
>>>>>
>>>>>       return bp->len;
>>>>>  }
>>>>>  @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h,
>>>>> AVBPrint
>>>>>  *pb, size_t max_size)
>>>>>           if (ret <= 0)
>>>>>               return ret;
>>>>>           av_bprint_append_data(pb, buf, ret);
>>>>>  -        if (!av_bprint_is_complete(pb))
>>>>>  +        if (!av_bprint_is_complete(pb)) {
>>>>>  +            h->error = AVERROR(ENOMEM);
>>>>>               return AVERROR(ENOMEM);
>>>>>  +        }
>>>>>           max_size -= ret;
>>>>>       }
>>>>>       return 0;
>>>>>
>>>>
>>>>  I don't really see the point of this: It is not a real read error that
>>>>  should stick to the AVIOContext (which can still be used afterwards
>>>>  without any issue).
>>>>  If the user does not check the errors, then the user
>>>>  has no one to blame but himself for missing errors.
>>>
>>> AVIO read/write behaviour is to store IO errors in the context so the
>>> user does not have to check for them in every call. It is not well
>>> documented which calls should be checked always, so the user might be
>>> under the impression that errors during read/write may be checked
>>> sometime later.
>>>
>>> Admittedly, ENOMEM is not an IO error, but I think it is better to
>>> store that as well in the context to keep the behaviour consistent,
>>> because in case of ENOMEM avio_read_to_bprint reads and drops
>>> undefined amount of data, so the context will also be in an undefined
>>> state.
>>>
>>> Other possibilities:
>>> - make avio_read_to_bprint read all the data regardless of AVBPrint
>>> fullness
>>>  - mark avio_read_to_bprint av_warn_unused_result.
>>>  - both :)
>>>
>>> But these also forces the user to check return values... So I kind of
>>> like my original approach better, because it maintains avio_read/write
>>> call behaviour that it is safe to check errors sometime later.
>> 
>> Any more comments about this or the rest of the series? I plan to apply
>> it tomorrow.
>> 
>
> I still don't like storing ENOMEM in the AVIOContext and I don't see why
> having to check the error is so burdensome. But if you want it so bad,
> then go ahead.

I don't feel strongly about it, it just looked convenient/consistent. But 
I will just skip this patch then and apply the rest.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".