diff mbox series

[v7,01/22] Revert "io: add new qio_channel_{readv, writev, read, write}_all functions"

Message ID 20170906115143.27451-2-quintela@redhat.com
State New
Headers show
Series Multifd | expand

Commit Message

Juan Quintela Sept. 6, 2017, 11:51 a.m. UTC
This reverts commit d4622e55883211072621958d39ddaa73483d201e.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/io/channel.h       |  90 ---------------------------------------
 io/channel.c               |  94 -----------------------------------------
 tests/io-channel-helpers.c | 102 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 93 insertions(+), 193 deletions(-)

Comments

Eric Blake Sept. 6, 2017, 2 p.m. UTC | #1
On 09/06/2017 06:51 AM, Juan Quintela wrote:
> This reverts commit d4622e55883211072621958d39ddaa73483d201e.

But with no reason why?  What bugs are you fixing by reverting this?

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/io/channel.h       |  90 ---------------------------------------
>  io/channel.c               |  94 -----------------------------------------
>  tests/io-channel-helpers.c | 102 +++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 93 insertions(+), 193 deletions(-)
>

Looking ahead, I see 8/22 recreates qio_channel_readv_all (but not
qio_channel_read_all); how does that differ from this one?

Should you be squashing 1/22 and 8/22 into a single non-revert patch
that just fixes bugs on top of what is already in the tree?

Also, have you seen my patches, that also fix bugs in the _all functions?
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01053.html
Juan Quintela Sept. 6, 2017, 2:42 p.m. UTC | #2
Eric Blake <eblake@redhat.com> wrote:
> On 09/06/2017 06:51 AM, Juan Quintela wrote:
>> This reverts commit d4622e55883211072621958d39ddaa73483d201e.
>
> But with no reason why?  What bugs are you fixing by reverting this?

I put it on the cover letter.  I am investigating *why* it fails on me.
It got the thread handed.

>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  include/io/channel.h       |  90 ---------------------------------------
>>  io/channel.c               |  94 -----------------------------------------
>>  tests/io-channel-helpers.c | 102 +++++++++++++++++++++++++++++++++++++++++----
>>  3 files changed, 93 insertions(+), 193 deletions(-)
>>
>
> Looking ahead, I see 8/22 recreates qio_channel_readv_all (but not
> qio_channel_read_all); how does that differ from this one?
>
> Should you be squashing 1/22 and 8/22 into a single non-revert patch
> that just fixes bugs on top of what is already in the tree?

My plan is to fix whatever is there and see why it is failing.

> Also, have you seen my patches, that also fix bugs in the _all functions?
> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01053.html

No, I have to take a look, thanks.

Thanks, Juan.
Eric Blake Sept. 6, 2017, 4:09 p.m. UTC | #3
On 09/06/2017 09:42 AM, Juan Quintela wrote:
> Eric Blake <eblake@redhat.com> wrote:
>> On 09/06/2017 06:51 AM, Juan Quintela wrote:
>>> This reverts commit d4622e55883211072621958d39ddaa73483d201e.
>>
>> But with no reason why?  What bugs are you fixing by reverting this?
> 
> I put it on the cover letter.  I am investigating *why* it fails on me.
> It got the thread handed.

Then I think I ran into the same problems as you (NBD was hanging for me
due to the nested event loop being called from a coroutine context),...

> 
> My plan is to fix whatever is there and see why it is failing.
> 
>> Also, have you seen my patches, that also fix bugs in the _all functions?
>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01053.html
> 
> No, I have to take a look, thanks.

...and hopefully my patch helps the hang you were seeing (now in a pull
request:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01338.html).
Daniel P. Berrangé Sept. 6, 2017, 4:32 p.m. UTC | #4
On Wed, Sep 06, 2017 at 04:42:18PM +0200, Juan Quintela wrote:
> Eric Blake <eblake@redhat.com> wrote:
> > On 09/06/2017 06:51 AM, Juan Quintela wrote:
> >> This reverts commit d4622e55883211072621958d39ddaa73483d201e.
> >
> > But with no reason why?  What bugs are you fixing by reverting this?
> 
> I put it on the cover letter.  I am investigating *why* it fails on me.
> It got the thread handed.

Your functions return the number of bytes written. My impl only
returns the 0 or -1 on the basis that the caller does not need
to know how many bytes were written - its always exactly the
amount asked for. You probably need to adjust your code to take
that into account.

Regards,
Daniel
diff mbox series

Patch

diff --git a/include/io/channel.h b/include/io/channel.h
index 8f25893c45..54f3dc252f 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -269,58 +269,6 @@  ssize_t qio_channel_writev_full(QIOChannel *ioc,
                                 Error **errp);
 
 /**
- * qio_channel_readv_all:
- * @ioc: the channel object
- * @iov: the array of memory regions to read data into
- * @niov: the length of the @iov array
- * @errp: pointer to a NULL-initialized error object
- *
- * Read data from the IO channel, storing it in the
- * memory regions referenced by @iov. Each element
- * in the @iov will be fully populated with data
- * before the next one is used. The @niov parameter
- * specifies the total number of elements in @iov.
- *
- * The function will wait for all requested data
- * to be read, yielding from the current coroutine
- * if required.
- *
- * If end-of-file occurs before all requested data
- * has been read, an error will be reported.
- *
- * Returns: 0 if all bytes were read, or -1 on error
- */
-int qio_channel_readv_all(QIOChannel *ioc,
-                          const struct iovec *iov,
-                          size_t niov,
-                          Error **errp);
-
-
-/**
- * qio_channel_writev_all:
- * @ioc: the channel object
- * @iov: the array of memory regions to write data from
- * @niov: the length of the @iov array
- * @errp: pointer to a NULL-initialized error object
- *
- * Write data to the IO channel, reading it from the
- * memory regions referenced by @iov. Each element
- * in the @iov will be fully sent, before the next
- * one is used. The @niov parameter specifies the
- * total number of elements in @iov.
- *
- * The function will wait for all requested data
- * to be written, yielding from the current coroutine
- * if required.
- *
- * Returns: 0 if all bytes were written, or -1 on error
- */
-int qio_channel_writev_all(QIOChannel *ioc,
-                           const struct iovec *iov,
-                           size_t niov,
-                           Error **erp);
-
-/**
  * qio_channel_readv:
  * @ioc: the channel object
  * @iov: the array of memory regions to read data into
@@ -383,44 +331,6 @@  ssize_t qio_channel_write(QIOChannel *ioc,
                           Error **errp);
 
 /**
- * qio_channel_read_all:
- * @ioc: the channel object
- * @buf: the memory region to read data into
- * @buflen: the number of bytes to @buf
- * @errp: pointer to a NULL-initialized error object
- *
- * Reads @buflen bytes into @buf, possibly blocking or (if the
- * channel is non-blocking) yielding from the current coroutine
- * multiple times until the entire content is read. If end-of-file
- * occurs it will return an error rather than a short-read. Otherwise
- * behaves as qio_channel_read().
- *
- * Returns: 0 if all bytes were read, or -1 on error
- */
-int qio_channel_read_all(QIOChannel *ioc,
-                         char *buf,
-                         size_t buflen,
-                         Error **errp);
-/**
- * qio_channel_write_all:
- * @ioc: the channel object
- * @buf: the memory region to write data into
- * @buflen: the number of bytes to @buf
- * @errp: pointer to a NULL-initialized error object
- *
- * Writes @buflen bytes from @buf, possibly blocking or (if the
- * channel is non-blocking) yielding from the current coroutine
- * multiple times until the entire content is written.  Otherwise
- * behaves as qio_channel_write().
- *
- * Returns: 0 if all bytes were written, or -1 on error
- */
-int qio_channel_write_all(QIOChannel *ioc,
-                          const char *buf,
-                          size_t buflen,
-                          Error **errp);
-
-/**
  * qio_channel_set_blocking:
  * @ioc: the channel object
  * @enabled: the blocking flag state
diff --git a/io/channel.c b/io/channel.c
index 5e8c2f0a91..1cfb8b33a2 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -22,7 +22,6 @@ 
 #include "io/channel.h"
 #include "qapi/error.h"
 #include "qemu/main-loop.h"
-#include "qemu/iov.h"
 
 bool qio_channel_has_feature(QIOChannel *ioc,
                              QIOChannelFeature feature)
@@ -86,79 +85,6 @@  ssize_t qio_channel_writev_full(QIOChannel *ioc,
 }
 
 
-
-int qio_channel_readv_all(QIOChannel *ioc,
-                          const struct iovec *iov,
-                          size_t niov,
-                          Error **errp)
-{
-    int ret = -1;
-    struct iovec *local_iov = g_new(struct iovec, niov);
-    struct iovec *local_iov_head = local_iov;
-    unsigned int nlocal_iov = niov;
-
-    nlocal_iov = iov_copy(local_iov, nlocal_iov,
-                          iov, niov,
-                          0, iov_size(iov, niov));
-
-    while (nlocal_iov > 0) {
-        ssize_t len;
-        len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
-        if (len == QIO_CHANNEL_ERR_BLOCK) {
-            qio_channel_wait(ioc, G_IO_IN);
-            continue;
-        } else if (len < 0) {
-            goto cleanup;
-        } else if (len == 0) {
-            error_setg(errp,
-                       "Unexpected end-of-file before all bytes were read");
-            goto cleanup;
-        }
-
-        iov_discard_front(&local_iov, &nlocal_iov, len);
-    }
-
-    ret = 0;
-
- cleanup:
-    g_free(local_iov_head);
-    return ret;
-}
-
-int qio_channel_writev_all(QIOChannel *ioc,
-                           const struct iovec *iov,
-                           size_t niov,
-                           Error **errp)
-{
-    int ret = -1;
-    struct iovec *local_iov = g_new(struct iovec, niov);
-    struct iovec *local_iov_head = local_iov;
-    unsigned int nlocal_iov = niov;
-
-    nlocal_iov = iov_copy(local_iov, nlocal_iov,
-                          iov, niov,
-                          0, iov_size(iov, niov));
-
-    while (nlocal_iov > 0) {
-        ssize_t len;
-        len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
-        if (len == QIO_CHANNEL_ERR_BLOCK) {
-            qio_channel_wait(ioc, G_IO_OUT);
-            continue;
-        }
-        if (len < 0) {
-            goto cleanup;
-        }
-
-        iov_discard_front(&local_iov, &nlocal_iov, len);
-    }
-
-    ret = 0;
- cleanup:
-    g_free(local_iov_head);
-    return ret;
-}
-
 ssize_t qio_channel_readv(QIOChannel *ioc,
                           const struct iovec *iov,
                           size_t niov,
@@ -197,26 +123,6 @@  ssize_t qio_channel_write(QIOChannel *ioc,
 }
 
 
-int qio_channel_read_all(QIOChannel *ioc,
-                         char *buf,
-                         size_t buflen,
-                         Error **errp)
-{
-    struct iovec iov = { .iov_base = buf, .iov_len = buflen };
-    return qio_channel_readv_all(ioc, &iov, 1, errp);
-}
-
-
-int qio_channel_write_all(QIOChannel *ioc,
-                          const char *buf,
-                          size_t buflen,
-                          Error **errp)
-{
-    struct iovec iov = { .iov_base = (char *)buf, .iov_len = buflen };
-    return qio_channel_writev_all(ioc, &iov, 1, errp);
-}
-
-
 int qio_channel_set_blocking(QIOChannel *ioc,
                               bool enabled,
                               Error **errp)
diff --git a/tests/io-channel-helpers.c b/tests/io-channel-helpers.c
index 5430e1389d..05e5579cf8 100644
--- a/tests/io-channel-helpers.c
+++ b/tests/io-channel-helpers.c
@@ -21,7 +21,6 @@ 
 #include "qemu/osdep.h"
 #include "io-channel-helpers.h"
 #include "qapi/error.h"
-#include "qemu/iov.h"
 
 struct QIOChannelTest {
     QIOChannel *src;
@@ -38,17 +37,77 @@  struct QIOChannelTest {
 };
 
 
+static void test_skip_iovec(struct iovec **iov,
+                            size_t *niov,
+                            size_t skip,
+                            struct iovec *old)
+{
+    size_t offset = 0;
+    size_t i;
+
+    for (i = 0; i < *niov; i++) {
+        if (skip < (*iov)[i].iov_len) {
+            old->iov_len = (*iov)[i].iov_len;
+            old->iov_base = (*iov)[i].iov_base;
+
+            (*iov)[i].iov_len -= skip;
+            (*iov)[i].iov_base += skip;
+            break;
+        } else {
+            skip -= (*iov)[i].iov_len;
+
+            if (i == 0 && old->iov_base) {
+                (*iov)[i].iov_len = old->iov_len;
+                (*iov)[i].iov_base = old->iov_base;
+                old->iov_len = 0;
+                old->iov_base = NULL;
+            }
+
+            offset++;
+        }
+    }
+
+    *iov = *iov + offset;
+    *niov -= offset;
+}
+
+
 /* This thread sends all data using iovecs */
 static gpointer test_io_thread_writer(gpointer opaque)
 {
     QIOChannelTest *data = opaque;
+    struct iovec *iov = data->inputv;
+    size_t niov = data->niov;
+    struct iovec old = { 0 };
 
     qio_channel_set_blocking(data->src, data->blocking, NULL);
 
-    qio_channel_writev_all(data->src,
-                           data->inputv,
-                           data->niov,
-                           &data->writeerr);
+    while (niov) {
+        ssize_t ret;
+        ret = qio_channel_writev(data->src,
+                                 iov,
+                                 niov,
+                                 &data->writeerr);
+        if (ret == QIO_CHANNEL_ERR_BLOCK) {
+            if (data->blocking) {
+                error_setg(&data->writeerr,
+                           "Unexpected I/O blocking");
+                break;
+            } else {
+                qio_channel_wait(data->src,
+                                 G_IO_OUT);
+                continue;
+            }
+        } else if (ret < 0) {
+            break;
+        } else if (ret == 0) {
+            error_setg(&data->writeerr,
+                       "Unexpected zero length write");
+            break;
+        }
+
+        test_skip_iovec(&iov, &niov, ret, &old);
+    }
 
     return NULL;
 }
@@ -58,13 +117,38 @@  static gpointer test_io_thread_writer(gpointer opaque)
 static gpointer test_io_thread_reader(gpointer opaque)
 {
     QIOChannelTest *data = opaque;
+    struct iovec *iov = data->outputv;
+    size_t niov = data->niov;
+    struct iovec old = { 0 };
 
     qio_channel_set_blocking(data->dst, data->blocking, NULL);
 
-    qio_channel_readv_all(data->dst,
-                          data->outputv,
-                          data->niov,
-                          &data->readerr);
+    while (niov) {
+        ssize_t ret;
+
+        ret = qio_channel_readv(data->dst,
+                                iov,
+                                niov,
+                                &data->readerr);
+
+        if (ret == QIO_CHANNEL_ERR_BLOCK) {
+            if (data->blocking) {
+                error_setg(&data->readerr,
+                           "Unexpected I/O blocking");
+                break;
+            } else {
+                qio_channel_wait(data->dst,
+                                 G_IO_IN);
+                continue;
+            }
+        } else if (ret < 0) {
+            break;
+        } else if (ret == 0) {
+            break;
+        }
+
+        test_skip_iovec(&iov, &niov, ret, &old);
+    }
 
     return NULL;
 }