| Message ID | 1364477297-2083-2-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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,