Re: [qemu-s390x] [PATCH v3 02/25] chardev: Assert IOCanReadHandler can not be negative

[Philippe Mathieu-Daudé]

On 2/20/19 11:03 AM, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 20, 2019 at 2:03 AM Philippe Mathieu-Daudé
> <address@hidden> wrote:
>>
>> The backend should not return a negative length to read.
>> We will later change the prototype of IOCanReadHandler to return an
>> unsigned length. Meanwhile make sure the return length is positive.
>>
>> Suggested-by: Paolo Bonzini <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> 
> In such patch, you should do extensive review of existing callbacks,
> or find a convincing argument that this can't break.

Argh I missed that.

> The problem is there are a lot of can_read callbacks, and it's not
> trivial. The *first* of git-grep is rng_egd_chr_can_read()
> 
>  57     QSIMPLEQ_FOREACH(req, &s->parent.requests, next) {
>  58         size += req->size - req->offset;
>  59     }
>  60
>  61     return size;
> 
> Clearly not obvious if it returns >= 0.
> 
> Another approach is to look at the caller and the return value
> handling. If none handle negative values (or would have wrong
> behaviour with negative values), the assert() is perhaps justified, as
> it could prevent from doing more harm.

I'll go and audit all of them.

Thanks for the review!

Phil.

>> ---
>>  chardev/char.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/chardev/char.c b/chardev/char.c
>> index f6d61fa5f8..71ecd32b25 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -159,12 +159,15 @@ int qemu_chr_write(Chardev *s, const uint8_t *buf, int 
>> len, bool write_all)
>>  int qemu_chr_be_can_write(Chardev *s)
>>  {
>>      CharBackend *be = s->be;
>> +    int receivable_bytes;
>>
>>      if (!be || !be->chr_can_read) {
>>          return 0;
>>      }
>>
>> -    return be->chr_can_read(be->opaque);
>> +    receivable_bytes = be->chr_can_read(be->opaque);
>> +    assert(receivable_bytes >= 0);
>> +    return receivable_bytes;
>>  }
>>
>>  void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len)
>> --
>> 2.20.1
>>