Message ID | 1373998781-29561-2-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
Laszlo Ersek <lersek@redhat.com> writes: > The g_io_channel_write_chars() documentation states, > > bytes_written: The number of bytes written. This can be nonzero even if > the return value is not G_IO_STATUS_NORMAL. [...] > > io_channel_send() could lose such bytes before. > > Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to > constant false whenever it was reached. When that condition actually held, > it always led to -1 / EINVAL. This patch (almost) distinguishes > G_IO_STATUS_EOF only when no bytes have been written, and then treats it > as an error. Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen if bytes_written > 0. I see what you mean by the comment but do you have any reason to believe this happens in practice? Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Regards, Anthony Liguori > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > qemu-char.c | 41 +++++++++++++++++++---------------------- > 1 files changed, 19 insertions(+), 22 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 800d6a6..c86ce4b 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -720,35 +720,32 @@ static GIOChannel *io_channel_from_socket(int fd) > > static int io_channel_send(GIOChannel *fd, const void *buf, size_t len) > { > - GIOStatus status; > - size_t offset; > + size_t offset = 0; > + GIOStatus status = G_IO_STATUS_NORMAL; > > - offset = 0; > - while (offset < len) { > - gsize bytes_written; > + while (offset < len && status == G_IO_STATUS_NORMAL) { > + gsize bytes_written = 0; > > status = g_io_channel_write_chars(fd, buf + offset, len - offset, > &bytes_written, NULL); > - if (status != G_IO_STATUS_NORMAL) { > - if (status == G_IO_STATUS_AGAIN) { > - /* If we've written any data, return a partial write. */ > - if (offset) { > - break; > - } > - errno = EAGAIN; > - } else { > - errno = EINVAL; > - } > - > - return -1; > - } else if (status == G_IO_STATUS_EOF) { > - break; > - } > - > offset += bytes_written; > } > > - return offset; > + if (offset > 0) { > + return offset; > + } > + switch (status) { > + case G_IO_STATUS_NORMAL: > + g_assert(len == 0); > + return 0; > + case G_IO_STATUS_AGAIN: > + errno = EAGAIN; > + return -1; > + default: > + break; > + } > + errno = EINVAL; > + return -1; > } > > #ifndef _WIN32 > -- > 1.7.1
On 07/16/13 20:57, Anthony Liguori wrote: > Laszlo Ersek <lersek@redhat.com> writes: > >> The g_io_channel_write_chars() documentation states, >> >> bytes_written: The number of bytes written. This can be nonzero even if >> the return value is not G_IO_STATUS_NORMAL. [...] >> >> io_channel_send() could lose such bytes before. >> >> Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to >> constant false whenever it was reached. When that condition actually held, >> it always led to -1 / EINVAL. This patch (almost) distinguishes >> G_IO_STATUS_EOF only when no bytes have been written, and then treats it >> as an error. > > Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen > if bytes_written > 0. I see what you mean by the comment but do you > have any reason to believe this happens in practice? In my opinion, G_IO_STATUS_EOF doesn't make any sense whatsoever for a write operation (for count>0) if glib kept any resemblance to write(), and should never happen in practice. The awkward commit message only captures the fact that I didn't forget about G_IO_STATUS_EOF, I considered it explicitly. > Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> Thanks! Laszlo
Laszlo Ersek <lersek@redhat.com> writes: > On 07/16/13 20:57, Anthony Liguori wrote: >> Laszlo Ersek <lersek@redhat.com> writes: >> >>> The g_io_channel_write_chars() documentation states, >>> >>> bytes_written: The number of bytes written. This can be nonzero even if >>> the return value is not G_IO_STATUS_NORMAL. [...] >>> >>> io_channel_send() could lose such bytes before. >>> >>> Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to >>> constant false whenever it was reached. When that condition actually held, >>> it always led to -1 / EINVAL. This patch (almost) distinguishes >>> G_IO_STATUS_EOF only when no bytes have been written, and then treats it >>> as an error. >> >> Just for my own benefit, I always assume G_IO_STATUS_EOF cannot happen >> if bytes_written > 0. I see what you mean by the comment but do you >> have any reason to believe this happens in practice? > > In my opinion, G_IO_STATUS_EOF doesn't make any sense whatsoever for a > write operation (for count>0) if glib kept any resemblance to write(), > and should never happen in practice. Okay, thanks! I'll give other folks a chance to look at this series and then apply in a day or so. Regards, Anthony Liguori > > The awkward commit message only captures the fact that I didn't forget > about G_IO_STATUS_EOF, I considered it explicitly. > >> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com> > > Thanks! > > Laszlo
diff --git a/qemu-char.c b/qemu-char.c index 800d6a6..c86ce4b 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -720,35 +720,32 @@ static GIOChannel *io_channel_from_socket(int fd) static int io_channel_send(GIOChannel *fd, const void *buf, size_t len) { - GIOStatus status; - size_t offset; + size_t offset = 0; + GIOStatus status = G_IO_STATUS_NORMAL; - offset = 0; - while (offset < len) { - gsize bytes_written; + while (offset < len && status == G_IO_STATUS_NORMAL) { + gsize bytes_written = 0; status = g_io_channel_write_chars(fd, buf + offset, len - offset, &bytes_written, NULL); - if (status != G_IO_STATUS_NORMAL) { - if (status == G_IO_STATUS_AGAIN) { - /* If we've written any data, return a partial write. */ - if (offset) { - break; - } - errno = EAGAIN; - } else { - errno = EINVAL; - } - - return -1; - } else if (status == G_IO_STATUS_EOF) { - break; - } - offset += bytes_written; } - return offset; + if (offset > 0) { + return offset; + } + switch (status) { + case G_IO_STATUS_NORMAL: + g_assert(len == 0); + return 0; + case G_IO_STATUS_AGAIN: + errno = EAGAIN; + return -1; + default: + break; + } + errno = EINVAL; + return -1; } #ifndef _WIN32
The g_io_channel_write_chars() documentation states, bytes_written: The number of bytes written. This can be nonzero even if the return value is not G_IO_STATUS_NORMAL. [...] io_channel_send() could lose such bytes before. Furthermore, the (status == G_IO_STATUS_EOF) condition used to evaluate to constant false whenever it was reached. When that condition actually held, it always led to -1 / EINVAL. This patch (almost) distinguishes G_IO_STATUS_EOF only when no bytes have been written, and then treats it as an error. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- qemu-char.c | 41 +++++++++++++++++++---------------------- 1 files changed, 19 insertions(+), 22 deletions(-)