[Qemu-devel] [PATCH v4 0/8] Migration: Remove copying of guest ram pages

[Qemu-devel] [PATCH v4 0/8] Migration: Remove copying of guest ram pages

[Orit Wasserman]

In migration all data is copied to a static buffer in QEMUFile,
this hurts our network bandwidth and CPU usage especially with large guests.
We switched to iovec for storing different buffers to send (even a byte field is
considered as a buffer) and use writev to send the iovec.
writev was chosen (as apposed to sendmsg) because it supprts non socket fds.
  
Guest memory pages are not copied by calling a new function 
qemu_put_buffer_no_copy.
The page header data and device state data are still copied into the static
buffer. This data consists of a lot of bytes and integer fields and the static
buffer is used to store it during batching.
Another improvement is changing qemu_putbe64/32/16 to create a single
buffer instead of several byte sized buffer.

git repository: git://github.com/oritwas/qemu.git sendv_v2

Changes from v3:
Use a variable for iov_size
Change f->bytes_xfer +=1 to f->bytes_xfer++
Remove unneeded "More optimized qemu_put_be64/32/16" patch
Move code to the right patch
Rename qemu_put_buffer_no_copy to qemu_put_buffer_async
Use function for updating the iovec that detect adjacent iovecs and coalesce
them.

Change from v2:
Always send data for the iovec even if writev_buffer is not implemented.
Coalesce adjacent iovecs to create one big buffer from small adjacent buffer.

Changes from v1:
Use iov_send for socket.
Make writev_buffer optional and if it is not implemented use put_buffer

Future work: Make number of iovec changeable

Orit Wasserman (8):
  Add QemuFileWritevBuffer QemuFileOps
  Add socket_writev_buffer function
  Update bytes_xfer in qemu_put_byte
  Store the data to send also in iovec
  Use writev ops if available
  Add qemu_put_buffer_async
  Use qemu_put_buffer_async for guest memory pages
  coalesce adjacent iovecs

 arch_init.c                   |   2 +-
 include/migration/qemu-file.h |  12 +++++
 savevm.c                      | 101 +++++++++++++++++++++++++++++++++++-------
 3 files changed, 99 insertions(+), 16 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v4 1/8] Add QemuFileWritevBuffer QemuFileOps

[Orit Wasserman]

This will allow us to write an iovec

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/qemu-file.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index df81261..8d3da9b 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -51,11 +51,18 @@ typedef int (QEMUFileCloseFunc)(void *opaque);
  */
 typedef int (QEMUFileGetFD)(void *opaque);
 
+/*
+ * This function writes an iovec to file.
+ */
+typedef int (QEMUFileWritevBufferFunc)(void *opaque, struct iovec *iov,
+                                       int iovcnt);
+
 typedef struct QEMUFileOps {
     QEMUFilePutBufferFunc *put_buffer;
     QEMUFileGetBufferFunc *get_buffer;
     QEMUFileCloseFunc *close;
     QEMUFileGetFD *get_fd;
+    QEMUFileWritevBufferFunc *writev_buffer;
 } QEMUFileOps;
 
 QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 2/8] Add socket_writev_buffer function

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 savevm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/savevm.c b/savevm.c
index 35c8d1e..6608b6e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -39,6 +39,7 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "qemu/bitops.h"
+#include "qemu/iov.h"
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
@@ -171,6 +172,19 @@ static void coroutine_fn yield_until_fd_readable(int fd)
     qemu_coroutine_yield();
 }
 
+static int socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt)
+{
+    QEMUFileSocket *s = opaque;
+    ssize_t len;
+    ssize_t size = iov_size(iov, iovcnt);
+
+    len = iov_send(s->fd, iov, iovcnt, 0, size);
+    if (len < size) {
+        len = -socket_error();
+    }
+    return len;
+}
+
 static int socket_get_fd(void *opaque)
 {
     QEMUFileSocket *s = opaque;
@@ -387,6 +401,7 @@ static const QEMUFileOps socket_read_ops = {
 static const QEMUFileOps socket_write_ops = {
     .get_fd =     socket_get_fd,
     .put_buffer = socket_put_buffer,
+    .writev_buffer = socket_writev_buffer,
     .close =      socket_close
 };
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 3/8] Update bytes_xfer in qemu_put_byte

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/savevm.c b/savevm.c
index 6608b6e..bd60169 100644
--- a/savevm.c
+++ b/savevm.c
@@ -648,6 +648,8 @@ void qemu_put_byte(QEMUFile *f, int v)
 
     f->buf[f->buf_index++] = v;
     f->is_write = 1;
+    f->bytes_xfer++;
+
     if (f->buf_index >= IO_BUF_SIZE) {
         qemu_fflush(f);
     }
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 4/8] Store the data to send also in iovec

[Orit Wasserman]

All data is still copied into the static buffer.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index bd60169..fb20f13 100644
--- a/savevm.c
+++ b/savevm.c
@@ -114,6 +114,7 @@ void qemu_announce_self(void)
 /* savevm/loadvm support */
 
 #define IO_BUF_SIZE 32768
+#define MAX_IOV_SIZE MIN(IOV_MAX, 64)
 
 struct QEMUFile {
     const QEMUFileOps *ops;
@@ -129,6 +130,9 @@ struct QEMUFile {
     int buf_size; /* 0 when writing */
     uint8_t buf[IO_BUF_SIZE];
 
+    struct iovec iov[MAX_IOV_SIZE];
+    unsigned int iovcnt;
+
     int last_error;
 };
 
@@ -528,6 +532,7 @@ static void qemu_fflush(QEMUFile *f)
             f->pos += f->buf_index;
         }
         f->buf_index = 0;
+        f->iovcnt = 0;
     }
     if (ret < 0) {
         qemu_file_set_error(f, ret);
@@ -620,12 +625,14 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         if (l > size)
             l = size;
         memcpy(f->buf + f->buf_index, buf, l);
+        f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
+        f->iov[f->iovcnt++].iov_len = l;
         f->is_write = 1;
         f->buf_index += l;
         f->bytes_xfer += l;
         buf += l;
         size -= l;
-        if (f->buf_index >= IO_BUF_SIZE) {
+        if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
             qemu_fflush(f);
             if (qemu_file_get_error(f)) {
                 break;
@@ -649,8 +656,10 @@ void qemu_put_byte(QEMUFile *f, int v)
     f->buf[f->buf_index++] = v;
     f->is_write = 1;
     f->bytes_xfer++;
+    f->iov[f->iovcnt].iov_base = f->buf + (f->buf_index - 1);
+    f->iov[f->iovcnt++].iov_len = 1;
 
-    if (f->buf_index >= IO_BUF_SIZE) {
+    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
         qemu_fflush(f);
     }
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 5/8] Use writev ops if available

[Orit Wasserman]

Update qemu_fflush and stdio_close to use writev ops if they are available
Use the buffers stored in the iovec.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/savevm.c b/savevm.c
index fb20f13..6c0cc7b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -293,7 +293,7 @@ static int stdio_fclose(void *opaque)
     QEMUFileStdio *s = opaque;
     int ret = 0;
 
-    if (s->file->ops->put_buffer) {
+    if (s->file->ops->put_buffer || s->file->ops->writev_buffer) {
         int fd = fileno(s->stdio_file);
         struct stat st;
 
@@ -516,24 +516,40 @@ static void qemu_file_set_error(QEMUFile *f, int ret)
     }
 }
 
-/** Flushes QEMUFile buffer
+/**
+ * Flushes QEMUFile buffer
  *
+ * If there is writev_buffer QEMUFileOps it uses it otherwise uses
+ * put_buffer ops.
  */
 static void qemu_fflush(QEMUFile *f)
 {
     int ret = 0;
+    int i = 0;
 
-    if (!f->ops->put_buffer) {
+    if (!f->ops->writev_buffer && !f->ops->put_buffer) {
         return;
     }
-    if (f->is_write && f->buf_index > 0) {
-        ret = f->ops->put_buffer(f->opaque, f->buf, f->pos, f->buf_index);
-        if (ret >= 0) {
-            f->pos += f->buf_index;
+
+    if (f->is_write && f->iovcnt > 0) {
+        if (f->ops->writev_buffer) {
+            ret = f->ops->writev_buffer(f->opaque, f->iov, f->iovcnt);
+            if (ret >= 0) {
+                f->pos += ret;
+            }
+        } else {
+            for (i = 0; i < f->iovcnt && ret >= 0; i++) {
+                ret = f->ops->put_buffer(f->opaque, f->iov[i].iov_base, f->pos,
+                                         f->iov[i].iov_len);
+                if (ret >= 0) {
+                    f->pos += ret;
+                }
+            }
         }
         f->buf_index = 0;
         f->iovcnt = 0;
     }
+
     if (ret < 0) {
         qemu_file_set_error(f, ret);
     }
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 6/8] Add qemu_put_buffer_async

[Orit Wasserman]

This allow us to add a buffer to the iovec to send without copying it
into the static buffer, the buffer will be send later when qemu_fflush is called.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 include/migration/qemu-file.h |  5 +++++
 savevm.c                      | 36 +++++++++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 8d3da9b..402408c 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f);
 int64_t qemu_ftell(QEMUFile *f);
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
 void qemu_put_byte(QEMUFile *f, int v);
+/*
+ * put_buffer without copying the buffer.
+ * The buffer should be available till it is sent asynchronously.
+ */
+void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size);
 
 static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
 {
diff --git a/savevm.c b/savevm.c
index 6c0cc7b..b024354 100644
--- a/savevm.c
+++ b/savevm.c
@@ -622,6 +622,29 @@ int qemu_fclose(QEMUFile *f)
     return ret;
 }
 
+void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
+{
+    if (f->last_error) {
+        return;
+    }
+
+    if (f->is_write == 0 && f->buf_index > 0) {
+        fprintf(stderr,
+                "Attempted to write to buffer while read buffer is not empty\n");
+        abort();
+    }
+
+    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
+    f->iov[f->iovcnt++].iov_len = size;
+
+    f->is_write = 1;
+    f->bytes_xfer += size;
+
+    if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
+        qemu_fflush(f);
+    }
+}
+
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 {
     int l;
@@ -641,19 +664,14 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         if (l > size)
             l = size;
         memcpy(f->buf + f->buf_index, buf, l);
-        f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
-        f->iov[f->iovcnt++].iov_len = l;
         f->is_write = 1;
         f->buf_index += l;
-        f->bytes_xfer += l;
+        qemu_put_buffer_async(f, f->buf + (f->buf_index - l), l);
+        if (qemu_file_get_error(f)) {
+            break;
+        }
         buf += l;
         size -= l;
-        if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
-            qemu_fflush(f);
-            if (qemu_file_get_error(f)) {
-                break;
-            }
-        }
     }
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 7/8] Use qemu_put_buffer_async for guest memory pages

[Orit Wasserman]

This will remove an unneeded copy of guest memory pages.
For the page header and device state we still copy the data to the
static buffer the other option is to allocate the memory on demand
which is more expensive.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 arch_init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index 98e2bc6..aa56a20 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -481,7 +481,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
             /* XBZRLE overflow or normal page */
             if (bytes_sent == -1) {
                 bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
-                qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
+                qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE);
                 bytes_sent += TARGET_PAGE_SIZE;
                 acct_info.norm_pages++;
             }
-- 
1.7.11.7

[Qemu-devel] [PATCH v4 8/8] coalesce adjacent iovecs

[Orit Wasserman]

This way we send one big buffer instead of many small ones

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 savevm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/savevm.c b/savevm.c
index b024354..9e8ddee 100644
--- a/savevm.c
+++ b/savevm.c
@@ -622,6 +622,18 @@ int qemu_fclose(QEMUFile *f)
     return ret;
 }
 
+static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
+{
+    /* check for adjoint buffer and colasce them */
+    if (f->iovcnt > 0 && buf == f->iov[f->iovcnt - 1].iov_base +
+        f->iov[f->iovcnt - 1].iov_len) {
+        f->iov[f->iovcnt - 1].iov_len += size;
+    } else {
+        f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
+        f->iov[f->iovcnt++].iov_len = size;
+    }
+}
+
 void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
 {
     if (f->last_error) {
@@ -634,8 +646,7 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, int size)
         abort();
     }
 
-    f->iov[f->iovcnt].iov_base = (uint8_t *)buf;
-    f->iov[f->iovcnt++].iov_len = size;
+    add_to_iovec(f, buf, size);
 
     f->is_write = 1;
     f->bytes_xfer += size;
@@ -690,8 +701,8 @@ void qemu_put_byte(QEMUFile *f, int v)
     f->buf[f->buf_index++] = v;
     f->is_write = 1;
     f->bytes_xfer++;
-    f->iov[f->iovcnt].iov_base = f->buf + (f->buf_index - 1);
-    f->iov[f->iovcnt++].iov_len = 1;
+
+    add_to_iovec(f, f->buf + (f->buf_index - 1), 1);
 
     if (f->buf_index >= IO_BUF_SIZE || f->iovcnt >= MAX_IOV_SIZE) {
         qemu_fflush(f);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v4 2/8] Add socket_writev_buffer function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On 03/21/2013 12:34 PM, Orit Wasserman wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  savevm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/savevm.c b/savevm.c
> index 35c8d1e..6608b6e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -39,6 +39,7 @@
>  #include "qmp-commands.h"
>  #include "trace.h"
>  #include "qemu/bitops.h"
> +#include "qemu/iov.h"
>  
>  #define SELF_ANNOUNCE_ROUNDS 5
>  
> @@ -171,6 +172,19 @@ static void coroutine_fn yield_until_fd_readable(int fd)
>      qemu_coroutine_yield();
>  }
>  
> +static int socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt)

Returning int...

> +{
> +    QEMUFileSocket *s = opaque;
> +    ssize_t len;
> +    ssize_t size = iov_size(iov, iovcnt);
> +
> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
> +    if (len < size) {
> +        len = -socket_error();
> +    }
> +    return len;

...but len is an ssize_t.  If we send an iov with 2 gigabytes of data,
this can wrap around to a negative int even though we send a positive
amount of data.  Why not make the callback be typed to return ssize_t
from the beginning (affects patch 1/8)?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 6/8] Add qemu_put_buffer_async

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

On 03/21/2013 12:34 PM, Orit Wasserman wrote:
> This allow us to add a buffer to the iovec to send without copying it

s/allow/allows/

> into the static buffer, the buffer will be send later when qemu_fflush is called.

s/send/sent/

> 
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  include/migration/qemu-file.h |  5 +++++
>  savevm.c                      | 36 +++++++++++++++++++++++++++---------
>  2 files changed, 32 insertions(+), 9 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 8/8] coalesce adjacent iovecs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On 03/21/2013 12:34 PM, Orit Wasserman wrote:
> This way we send one big buffer instead of many small ones
> 
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  savevm.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index b024354..9e8ddee 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -622,6 +622,18 @@ int qemu_fclose(QEMUFile *f)
>      return ret;
>  }
>  
> +static void add_to_iovec(QEMUFile *f, const uint8_t *buf, int size)
> +{
> +    /* check for adjoint buffer and colasce them */

s/adjoint/adjacent/
s/colasce/coalesce/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 2/8] Add socket_writev_buffer function

[Orit Wasserman]

On 03/21/2013 09:35 PM, Eric Blake wrote:
> On 03/21/2013 12:34 PM, Orit Wasserman wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  savevm.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 35c8d1e..6608b6e 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -39,6 +39,7 @@
>>  #include "qmp-commands.h"
>>  #include "trace.h"
>>  #include "qemu/bitops.h"
>> +#include "qemu/iov.h"
>>  
>>  #define SELF_ANNOUNCE_ROUNDS 5
>>  
>> @@ -171,6 +172,19 @@ static void coroutine_fn yield_until_fd_readable(int fd)
>>      qemu_coroutine_yield();
>>  }
>>  
>> +static int socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt)
> 
> Returning int...
> 
>> +{
>> +    QEMUFileSocket *s = opaque;
>> +    ssize_t len;
>> +    ssize_t size = iov_size(iov, iovcnt);
>> +
>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>> +    if (len < size) {
>> +        len = -socket_error();
>> +    }
>> +    return len;
> 
> ...but len is an ssize_t.  If we send an iov with 2 gigabytes of data,
> this can wrap around to a negative int even though we send a positive
> amount of data.  Why not make the callback be typed to return ssize_t
> from the beginning (affects patch 1/8)?
At the moment it is not an issue but for the future we need to switch to ssize_t 
instead on int, I will change it.
We actually need to replace it all around the migration code but this should
be done in a different patch series.

Orit

> 

Re: [Qemu-devel] [PATCH v4 2/8] Add socket_writev_buffer function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

On 03/22/2013 01:30 AM, Orit Wasserman wrote:

>>>  
>>> +static int socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt)
>>
>> Returning int...
>>
>>> +{
>>> +    QEMUFileSocket *s = opaque;
>>> +    ssize_t len;
>>> +    ssize_t size = iov_size(iov, iovcnt);
>>> +
>>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>>> +    if (len < size) {
>>> +        len = -socket_error();
>>> +    }
>>> +    return len;
>>
>> ...but len is an ssize_t.  If we send an iov with 2 gigabytes of data,
>> this can wrap around to a negative int even though we send a positive
>> amount of data.  Why not make the callback be typed to return ssize_t
>> from the beginning (affects patch 1/8)?
> At the moment it is not an issue but for the future we need to switch to ssize_t 
> instead on int, I will change it.
> We actually need to replace it all around the migration code but this should
> be done in a different patch series.

I agree that the existing code base is in horrible shape with regards to
int instead of ssize_t, and that it will take a different patch series
to clean that up.  But why make that future patch harder?  New
interfaces might as well be designed correctly, to limit the cleanup to
the old interfaces, instead of making the cleanup job even harder.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v4 2/8] Add socket_writev_buffer function

[Orit Wasserman]

On 03/22/2013 07:08 PM, Eric Blake wrote:
> On 03/22/2013 01:30 AM, Orit Wasserman wrote:
> 
>>>>  
>>>> +static int socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt)
>>>
>>> Returning int...
>>>
>>>> +{
>>>> +    QEMUFileSocket *s = opaque;
>>>> +    ssize_t len;
>>>> +    ssize_t size = iov_size(iov, iovcnt);
>>>> +
>>>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>>>> +    if (len < size) {
>>>> +        len = -socket_error();
>>>> +    }
>>>> +    return len;
>>>
>>> ...but len is an ssize_t.  If we send an iov with 2 gigabytes of data,
>>> this can wrap around to a negative int even though we send a positive
>>> amount of data.  Why not make the callback be typed to return ssize_t
>>> from the beginning (affects patch 1/8)?
>> At the moment it is not an issue but for the future we need to switch to ssize_t 
>> instead on int, I will change it.
>> We actually need to replace it all around the migration code but this should
>> be done in a different patch series.
> 
> I agree that the existing code base is in horrible shape with regards to
> int instead of ssize_t, and that it will take a different patch series
> to clean that up.  But why make that future patch harder?  New
> interfaces might as well be designed correctly, to limit the cleanup to
> the old interfaces, instead of making the cleanup job even harder.
> 
I agree completely! new interface should be designed correctly.