Message ID | 1348217255-22441-26-git-send-email-quintela@redhat.com |
---|---|
State | New |
Headers | show |
Il 21/09/2012 10:47, Juan Quintela ha scritto: > Or the amount of data written if there is no error. Adjust all callers. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > buffered_file.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/buffered_file.c b/buffered_file.c > index 747d672..9db73dc 100644 > --- a/buffered_file.c > +++ b/buffered_file.c > @@ -58,26 +58,26 @@ static void buffered_append(QEMUFileBuffered *s, > s->buffer_size += size; > } > > -static void buffered_flush(QEMUFileBuffered *s) > +static int buffered_flush(QEMUFileBuffered *s) Should this be ssize_t? > { > size_t offset = 0; > + int ret = 0; > > DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size); > > while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) { > - ssize_t ret; > > ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset, > s->buffer_size - offset); > if (ret == -EAGAIN) { > DPRINTF("backend not ready, freezing\n"); > + ret = 0; > s->freeze_output = 1; > break; > } > > if (ret <= 0) { > DPRINTF("error flushing data, %zd\n", ret); > - qemu_file_set_error(s->file, ret); > break; > } else { > DPRINTF("flushed %zd byte(s)\n", ret); > @@ -89,6 +89,11 @@ static void buffered_flush(QEMUFileBuffered *s) > DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size); > memmove(s->buffer, s->buffer + offset, s->buffer_size - offset); > s->buffer_size -= offset; > + > + if (ret < 0) { > + return ret; > + } > + return offset; > } > > static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size) > @@ -112,7 +117,13 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in > buffered_append(s, buf, size); > } > > - buffered_flush(s); > + error = buffered_flush(s); > + if (error < 0) { > + DPRINTF("buffered flush error. bailing: %s\n", strerror(-error)); > + qemu_file_set_error(s->file, error); > + > + return error; > + } > > if (pos == 0 && size == 0) { > DPRINTF("file is ready\n"); > @@ -128,19 +139,24 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in > static int buffered_close(void *opaque) > { > QEMUFileBuffered *s = opaque; > - int ret; > + int ret = 0, ret2; Same here too (ssize_t). Paolo > > DPRINTF("closing\n"); > > s->xfer_limit = INT_MAX; > while (!qemu_file_get_error(s->file) && s->buffer_size) { > - buffered_flush(s); > + ret = buffered_flush(s); > + if (ret < 0) { > + break; > + } > if (s->freeze_output) > migrate_fd_wait_for_unfreeze(s->migration_state); > } > > - ret = migrate_fd_close(s->migration_state); > - > + ret2 = migrate_fd_close(s->migration_state); > + if (ret >= 0) { > + ret = ret2; > + } Perhaps the other way round: if (ret < 0) { ret2 = -1; } ... return ret2; so that buffered_close can still return int without having a suspicious truncation of its return value from ssize_t to int. Paolo > qemu_del_timer(s->timer); > qemu_free_timer(s->timer); > g_free(s->buffer); >
Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 21/09/2012 10:47, Juan Quintela ha scritto: >> Or the amount of data written if there is no error. Adjust all callers. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> buffered_file.c | 32 ++++++++++++++++++++++++-------- >> 1 file changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/buffered_file.c b/buffered_file.c >> index 747d672..9db73dc 100644 >> --- a/buffered_file.c >> +++ b/buffered_file.c >> @@ -58,26 +58,26 @@ static void buffered_append(QEMUFileBuffered *s, >> s->buffer_size += size; >> } >> >> -static void buffered_flush(QEMUFileBuffered *s) >> +static int buffered_flush(QEMUFileBuffered *s) > > Should this be ssize_t? Done. > Perhaps the other way round: > > if (ret < 0) { > ret2 = -1; > } > ... > return ret2; This lost the 1st errno value. The other way around we preserve it. Later, Juan.
Il 02/10/2012 13:06, Juan Quintela ha scritto: >> Perhaps the other way round: >> > >> > if (ret < 0) { >> > ret2 = -1; >> > } >> > ... >> > return ret2; > This lost the 1st errno value. Right, I meant ret2 = ret; > The other way around we preserve it. I think with "ret2 = ret;" it is the same. The way you have it in your patch, you could truncate a positive ssize_t return value from buffered_flush that does not fit in an int. Theoretical, I know, but I can see Coverity spotting it from a mile... Paolo
diff --git a/buffered_file.c b/buffered_file.c index 747d672..9db73dc 100644 --- a/buffered_file.c +++ b/buffered_file.c @@ -58,26 +58,26 @@ static void buffered_append(QEMUFileBuffered *s, s->buffer_size += size; } -static void buffered_flush(QEMUFileBuffered *s) +static int buffered_flush(QEMUFileBuffered *s) { size_t offset = 0; + int ret = 0; DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size); while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) { - ssize_t ret; ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset, s->buffer_size - offset); if (ret == -EAGAIN) { DPRINTF("backend not ready, freezing\n"); + ret = 0; s->freeze_output = 1; break; } if (ret <= 0) { DPRINTF("error flushing data, %zd\n", ret); - qemu_file_set_error(s->file, ret); break; } else { DPRINTF("flushed %zd byte(s)\n", ret); @@ -89,6 +89,11 @@ static void buffered_flush(QEMUFileBuffered *s) DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size); memmove(s->buffer, s->buffer + offset, s->buffer_size - offset); s->buffer_size -= offset; + + if (ret < 0) { + return ret; + } + return offset; } static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size) @@ -112,7 +117,13 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in buffered_append(s, buf, size); } - buffered_flush(s); + error = buffered_flush(s); + if (error < 0) { + DPRINTF("buffered flush error. bailing: %s\n", strerror(-error)); + qemu_file_set_error(s->file, error); + + return error; + } if (pos == 0 && size == 0) { DPRINTF("file is ready\n"); @@ -128,19 +139,24 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in static int buffered_close(void *opaque) { QEMUFileBuffered *s = opaque; - int ret; + int ret = 0, ret2; DPRINTF("closing\n"); s->xfer_limit = INT_MAX; while (!qemu_file_get_error(s->file) && s->buffer_size) { - buffered_flush(s); + ret = buffered_flush(s); + if (ret < 0) { + break; + } if (s->freeze_output) migrate_fd_wait_for_unfreeze(s->migration_state); } - ret = migrate_fd_close(s->migration_state); - + ret2 = migrate_fd_close(s->migration_state); + if (ret >= 0) { + ret = ret2; + } qemu_del_timer(s->timer); qemu_free_timer(s->timer); g_free(s->buffer);
Or the amount of data written if there is no error. Adjust all callers. Signed-off-by: Juan Quintela <quintela@redhat.com> --- buffered_file.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)