Re: [Qemu-devel] [PATCH 1/7] virtio-console: Also throttle when less was written then requested

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 03/29/2013 10:31 AM, Amit Shah wrote:
> On (Thu) 28 Mar 2013 [14:28:11], Hans de Goede wrote:
>> This is necessary so that we get properly woken up to write the rest.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Amit Shah <amit.shah@redhat.com>
>> ---
>>   hw/virtio-console.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>> index 284180f..015947c 100644
>> --- a/hw/virtio-console.c
>> +++ b/hw/virtio-console.c
>> @@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>>       ret = qemu_chr_fe_write(vcon->chr, buf, len);
>>       trace_virtio_console_flush_buf(port->id, len, ret);
>>
>> -    if (ret <= 0) {
>> +    if (ret < len) {
>
> Oops, there's a conversion bug here: len is size_t, and ret is
> ssize_t.  Both need to be the same type.
>
> Since we're not returning negative, ret should be changed to size_t.
> The function signature changes too, but that's alright.

Erm changing ret to a size_t will break things, since qemu_chr_fe_write can
return < 0 and storing that into an unsigned will change it into a very
large value instead ...

A better fix would be to change len to ssize_t. Although that will cause
issues if we ever get a single buf which is larger then 2^31 - 1. Note
we already have that problem since qemu_chr_fe_write already cannot handle
such large buffers...


>
>>           VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>>
>>           /*
>> @@ -56,7 +56,8 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>>            * we had a finer-grained message, like -EPIPE, we could close
>>            * this connection.
>>            */
>> -        ret = 0;
>> +        if (ret < 0)
>> +            ret = 0;
>
> And there need to be braces here.

Ack.

Regards,

Hans