Patchwork [1/7] virtio-console: Also throttle when less was written then requested

login
register
mail settings
Submitter Hans de Goede
Date March 28, 2013, 1:28 p.m.
Message ID <1364477297-2083-2-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/232007/
State New
Headers show

Comments

Hans de Goede - March 28, 2013, 1:28 p.m.
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(-)
Amit Shah - March 29, 2013, 9:31 a.m.
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.

>          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.

		Amit
Hans de Goede - April 1, 2013, 4 p.m.
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
Amit Shah - April 2, 2013, 10:40 a.m.
On (Mon) 01 Apr 2013 [18:00:55], Hans de Goede wrote:
> 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 ...

Oh, definitely.  I meant changing the return type of flush_buf(), but
that doesn't help matters much...

> 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...

Yep.  Do you want to do that as part of this series?

		Amit
Hans de Goede - April 2, 2013, 5:32 p.m.
Hi,

On 04/02/2013 12:40 PM, Amit Shah wrote:
> On (Mon) 01 Apr 2013 [18:00:55], Hans de Goede wrote:
>> 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 ...
>
> Oh, definitely.  I meant changing the return type of flush_buf(), but
> that doesn't help matters much...
>
>> 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...
>
> Yep.  Do you want to do that as part of this series?

Yes, I'll send a v2 of this patch incorporating the change for Gerd to
pick up.

Regards,

Hans

Patch

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) {
         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;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
             qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,