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

[Cédric Le Goater]

On 3/4/24 22:04, Fabiano Rosas wrote:
> 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;
>          }
>          ...


Indeed.

I understand that the bdrv_nb_sectors() == 0 case only breaks the loop but
it is not considered as an error. Could the block folks confirm please ?

Thanks,

C.

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