Patchwork [v4,2/8] Add socket_writev_buffer function

login
register
mail settings
Submitter Orit Wasserman
Date March 21, 2013, 6:34 p.m.
Message ID <1363890878-8161-3-git-send-email-owasserm@redhat.com>
Download mbox | patch
Permalink /patch/229800/
State New
Headers show

Comments

Orit Wasserman - March 21, 2013, 6:34 p.m.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 savevm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
Eric Blake - March 21, 2013, 7:35 p.m.
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)?
Orit Wasserman - March 22, 2013, 7:30 a.m.
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

>
Eric Blake - March 22, 2013, 5:08 p.m.
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.
Orit Wasserman - March 23, 2013, 7:07 a.m.
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.

Patch

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
 };