[38/41] block-migration: handle errors with the return codes correctly

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block-migration.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Il 21/09/2012 10:47, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  block-migration.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 565628f..9f94733 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -423,10 +423,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
> 
>  error:
>      DPRINTF("Error reading sector %" PRId64 "\n", sector);
> -    qemu_file_set_error(f, ret);
>      g_free(blk->buf);
>      g_free(blk);
> -    return 0;
> +    return ret;
>  }
> 
>  /* return value:
> @@ -440,7 +439,7 @@ static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
> 
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>          ret = mig_save_device_dirty(f, bmds, is_async);
> -        if (ret == 0) {
> +        if (ret <= 0) {
>              break;
>          }
>      }
> @@ -598,12 +597,17 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>                  block_mig_state.bulk_completed = 1;
>              }
>          } else {
> -            if (blk_mig_save_dirty_block(f, 1) != 0) {
> +            ret = blk_mig_save_dirty_block(f, 1);
> +            if (ret != 0) {
>                  /* no more dirty blocks */
>                  break;
>              }
>          }
>      }
> +    if (ret) {
> +        blk_mig_cleanup();
> +        return ret;
> +    }
> 
>      ret = flush_blks(f);
>      if (ret) {
> @@ -635,18 +639,15 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>         all async read completed */
>      assert(block_mig_state.submitted == 0);
> 

Not clear what the fixes are, I'll take it as a cleanup.  Then:

> -    while (blk_mig_save_dirty_block(f, 0) == 0) {
> +    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
>          /* Do nothing */
>      }

use a do...while instead:

    do {
        ret = blk_mig_save_dirty_block(f, 0);
    } while (ret == 0);

>      blk_mig_cleanup();
> -
> -    /* report completion */
> -    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
> -
> -    ret = qemu_file_get_error(f);
>      if (ret) {
>          return ret;
>      }

Is it correct that we now return 1?  We used to return 0.

Paolo

> +    /* report completion */
> +    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
> 
>      DPRINTF("Block migration completed\n");
>

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 10:47, Juan Quintela ha scritto:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> @@ -635,18 +639,15 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>>         all async read completed */
>>      assert(block_mig_state.submitted == 0);
>> 
>
> Not clear what the fixes are, I'll take it as a cleanup.  Then:

We are returning now the errors directly.  Until now, we were 

>
>> -    while (blk_mig_save_dirty_block(f, 0) == 0) {
>> +    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
>>          /* Do nothing */
>>      }
>
> use a do...while instead:
>
>     do {
>         ret = blk_mig_save_dirty_block(f, 0);
>     } while (ret == 0);

I wanted to minimize the changes, but I don't really care, 

>
>>      blk_mig_cleanup();
>> -
>> -    /* report completion */
>> -    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);
>> -
>> -    ret = qemu_file_get_error(f);
>>      if (ret) {
>>          return ret;
>>      }
>
> Is it correct that we now return 1?  We used to return 0.

If you look at the whole change, old code was:


    while (blk_mig_save_dirty_block(f, 0) != 0) {
        /* Do nothing */
    }
    blk_mig_cleanup();

    /* report completion */
    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);

    ret = qemu_file_get_error(f);
    if (ret) {
        return ret;
    }

    DPRINTF("Block migration completed\n");

    qemu_put_be64(f, BLK_MIG_FLAG_EOS);

    return 0;

i.e. if there is one error, we return it, otherwise, we return 0.

    while ((ret = blk_mig_save_dirty_block(f, 0)) == 0) {
        /* Do nothing */
    }
    blk_mig_cleanup();
    if (ret) {
        return ret;
    }
    /* report completion */
    qemu_put_be64(f, (100 << BDRV_SECTOR_BITS) | BLK_MIG_FLAG_PROGRESS);

    DPRINTF("Block migration completed\n");

    qemu_put_be64(f, BLK_MIG_FLAG_EOS);

    return 0;

Now, we give exactly the same error, just that we return the error
directly from blk_mig_save_dirty_block() instead of storing it on
QEMUFile and getting it through qemu_file_get_error().

Notice that between old code and the _newer_ code, we have reverted the
return value of blk_mig_save_block to make things more consistent.

Later, Juan.

Il 24/09/2012 20:24, Juan Quintela ha scritto:
> Now, we give exactly the same error, just that we return the error
> directly from blk_mig_save_dirty_block() instead of storing it on
> QEMUFile and getting it through qemu_file_get_error().

Right you are.  Apart from the do/while,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>