| Message ID | 1363890878-8161-3-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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)?
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 >
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.
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.
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 };
Signed-off-by: Orit Wasserman <owasserm@redhat.com> --- savevm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)