| Message ID | 20240304122844.1888308-4-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Improve error reporting | expand |
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);
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); >
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(); @@ -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);
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(-)