Re: [PATCH v3 03/26] migration: Always report an error in block_save_setup()

[Fabiano Rosas <farosas@suse.de>]

Cédric Le Goater <clg@redhat.com> writes:

> This will prepare ground for futur changes adding an Error** argument
> to the save_setup() handler. We need to make sure that on failure,
> block_save_setup() always sets a new error.
>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  migration/block.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/migration/block.c b/migration/block.c
> index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..06f5857cf049df3261d2cf1d7c3607ab92350ac6 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -367,7 +367,7 @@ static void unset_dirty_tracking(void)
>      }
>  }
>  
> -static int init_blk_migration(QEMUFile *f)
> +static int init_blk_migration(QEMUFile *f, Error **errp)
>  {
>      BlockDriverState *bs;
>      BlkMigDevState *bmds;
> @@ -378,7 +378,6 @@ static int init_blk_migration(QEMUFile *f)
>          BlkMigDevState *bmds;
>          BlockDriverState *bs;
>      } *bmds_bs;
> -    Error *local_err = NULL;
>      int ret;
>  
>      GRAPH_RDLOCK_GUARD_MAINLOOP();

There's a negative return below at:

    for (i = 0, bs = bdrv_first(&it); bs; bs = bdrv_next(&it), i++) {
        if (bdrv_is_read_only(bs)) {
            continue;
        }

        sectors = bdrv_nb_sectors(bs);
        if (sectors <= 0) {
            ret = sectors;
                ^
            bdrv_next_cleanup(&it);
            goto out;
        }
        ...

I presume that must be covered by an error as well. A similar function
somewhere else reports:

        total_sectors = blk_nb_sectors(blk);
        if (total_sectors <= 0) {
            error_report("Error getting length of block device %s",
                         device_name);
            return -EINVAL;
        }

> @@ -439,9 +438,8 @@ static int init_blk_migration(QEMUFile *f)
>          bs = bmds_bs[i].bs;
>  
>          if (bmds) {
> -            ret = blk_insert_bs(bmds->blk, bs, &local_err);
> +            ret = blk_insert_bs(bmds->blk, bs, errp);
>              if (ret < 0) {
> -                error_report_err(local_err);
>                  goto out;
>              }
>  
> @@ -711,6 +709,7 @@ static void block_migration_cleanup(void *opaque)
>  static int block_save_setup(QEMUFile *f, void *opaque)
>  {
>      int ret;
> +    Error *local_err = NULL;
>  
>      trace_migration_block_save("setup", block_mig_state.submitted,
>                                 block_mig_state.transferred);
> @@ -718,18 +717,27 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>      warn_report("block migration is deprecated;"
>                  " use blockdev-mirror with NBD instead");
>  
> -    ret = init_blk_migration(f);
> +    ret = init_blk_migration(f, &local_err);
>      if (ret < 0) {
> +        error_report_err(local_err);
>          return ret;
>      }
>  
>      /* start track dirty blocks */
>      ret = set_dirty_tracking();
>      if (ret) {
> +        error_setg_errno(&local_err, -ret,
> +                         "Failed to start block dirty tracking");
> +        error_report_err(local_err);
>          return ret;
>      }
>  
>      ret = flush_blks(f);
> +    if (ret) {
> +        error_setg_errno(&local_err, -ret, "Flushing block failed");
> +        error_report_err(local_err);
> +        return ret;
> +    }
>      blk_mig_reset_dirty_cursor();
>      qemu_put_be64(f, BLK_MIG_FLAG_EOS);