[25/41] buffered_file: make buffered_flush return the error code

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(-)

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