Re: [Qemu-devel] [PATCH v5 03/17] qio: Create new qio_channel_{readv, writev}_all

[Juan Quintela <quintela@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Jul 17, 2017 at 03:42:24PM +0200, Juan Quintela wrote:
>> The functions waits until it is able to write the full iov.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> --
>> 
>> Add tests.
>> ---
>>  include/io/channel.h           | 46 +++++++++++++++++++++++++
>>  io/channel.c                   | 76 ++++++++++++++++++++++++++++++++++++++++++
>>  migration/qemu-file-channel.c  | 29 +---------------
>>  tests/io-channel-helpers.c     | 55 ++++++++++++++++++++++++++++++
>>  tests/io-channel-helpers.h     |  4 +++
>>  tests/test-io-channel-buffer.c | 55 ++++++++++++++++++++++++++++--
>>  6 files changed, 234 insertions(+), 31 deletions(-)
>
>
>
>> diff --git a/io/channel.c b/io/channel.c
>> index cdf7454..82203ef 100644
>> --- a/io/channel.c
>> +++ b/io/channel.c
>> @@ -22,6 +22,7 @@
>>  #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)
>> @@ -85,6 +86,81 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>>  }
>>  
>>  
>> +
>> +ssize_t qio_channel_readv_all(QIOChannel *ioc,
>> +                              const struct iovec *iov,
>> +                              size_t niov,
>> +                              Error **errp)
>> +{
>> +    ssize_t done = 0;
>> +    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_OUT);
>> +            continue;
>> +        }
>> +        if (len < 0) {
>> +            error_setg_errno(errp, EIO,
>> +                             "Channel was not able to read full iov");
>> +            done = -1;
>> +            goto cleanup;
>> +        }
>> +
>> +        iov_discard_front(&local_iov, &nlocal_iov, len);
>> +        done += len;
>> +    }
>
> If 'len == 0' (ie EOF from qio_channel_readv())  then this will busy
> loop. You need to break the loop on that condition and return whatever
> 'done' currently is.

Done.

>> +static void test_io_channel_buf2(void)
>> +{
>> +    QIOChannelBuffer *buf;
>> +    QIOChannelTest *test;
>> +
>> +    buf = qio_channel_buffer_new(0);
>> +
>> +    test = qio_channel_test_new();
>> +    qio_channel_test_run_writer_all(test, QIO_CHANNEL(buf));
>> +    buf->offset = 0;
>> +    qio_channel_test_run_reader(test, QIO_CHANNEL(buf));
>> +    qio_channel_test_validate(test);
>> +
>> +    object_unref(OBJECT(buf));
>> +}
>> +
>> +static void test_io_channel_buf3(void)
>> +{
>> +    QIOChannelBuffer *buf;
>> +    QIOChannelTest *test;
>> +
>> +    buf = qio_channel_buffer_new(0);
>> +
>> +    test = qio_channel_test_new();
>> +    qio_channel_test_run_writer(test, QIO_CHANNEL(buf));
>> +    buf->offset = 0;
>> +    qio_channel_test_run_reader_all(test, QIO_CHANNEL(buf));
>> +    qio_channel_test_validate(test);
>> +
>> +    object_unref(OBJECT(buf));
>> +}
>> +
>> +static void test_io_channel_buf4(void)
>> +{
>> +    QIOChannelBuffer *buf;
>> +    QIOChannelTest *test;
>> +
>> +    buf = qio_channel_buffer_new(0);
>> +
>> +    test = qio_channel_test_new();
>> +    qio_channel_test_run_writer_all(test, QIO_CHANNEL(buf));
>> +    buf->offset = 0;
>> +    qio_channel_test_run_reader_all(test, QIO_CHANNEL(buf));
>> +    qio_channel_test_validate(test);
>> +
>> +    object_unref(OBJECT(buf));
>> +}
>>  
>>  int main(int argc, char **argv)
>>  {
>> @@ -46,6 +92,9 @@ int main(int argc, char **argv)
>>  
>>      g_test_init(&argc, &argv, NULL);
>>  
>> -    g_test_add_func("/io/channel/buf", test_io_channel_buf);
>> +    g_test_add_func("/io/channel/buf1", test_io_channel_buf1);
>> +    g_test_add_func("/io/channel/buf2", test_io_channel_buf2);
>> +    g_test_add_func("/io/channel/buf3", test_io_channel_buf3);
>> +    g_test_add_func("/io/channel/buf4", test_io_channel_buf4);
>>      return g_test_run();
>>  }
>
> There's no need to add any of these additions to the test suite.  Instead
> you can just change the existing io-channel-helpers.c functions
> test_io_thread_writer() and test_io_thread_reader(), to call
> qio_channel_writev_all() & qio_channel_readv_all() respectively.

They are already done now, and the advantage of this was that I was able
to test that everything worked well against everything.  That was good
to be able to check that all worked as expected.

Later, Juan.