| Message ID | 20240306133441.2351700-8-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Improve error reporting | expand |
Cédric Le Goater <clg@redhat.com> writes: > This will prepare ground for future 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> Reviewed-by: Fabiano Rosas <farosas@suse.de>
On Wed, Mar 06, 2024 at 02:34:22PM +0100, Cédric Le Goater wrote: > @@ -404,6 +403,10 @@ static int init_blk_migration(QEMUFile *f) > sectors = bdrv_nb_sectors(bs); > if (sectors <= 0) { Not directly relevant to this patch, but just to mention that this looks suspicious (even if I know nothing about block migration..) - I am not sure whether any block drive would return 0 here, if so it looks still like a problem if we do the cleanup, ignoring the rest and return a success. > ret = sectors; > + if (ret < 0) { > + error_setg(errp, "Error getting length of block device %s", > + bdrv_get_device_name(bs)); > + } > bdrv_next_cleanup(&it); > goto out; > }
On 3/8/24 07:59, Peter Xu wrote: > On Wed, Mar 06, 2024 at 02:34:22PM +0100, Cédric Le Goater wrote: >> @@ -404,6 +403,10 @@ static int init_blk_migration(QEMUFile *f) >> sectors = bdrv_nb_sectors(bs); >> if (sectors <= 0) { > > Not directly relevant to this patch, but just to mention that this looks > suspicious (even if I know nothing about block migration..) - I am not sure > whether any block drive would return 0 here, if so it looks still like a > problem if we do the cleanup, ignoring the rest and return a success. yes and it is not symmetric with block_load() : total_sectors = blk_nb_sectors(blk); if (total_sectors <= 0) { error_report("Error getting length of block device %s", device_name); return -EINVAL; } > >> ret = sectors; >> + if (ret < 0) { >> + error_setg(errp, "Error getting length of block device %s", >> + bdrv_get_device_name(bs)); >> + } >> bdrv_next_cleanup(&it); >> goto out; >> } > May be Kevin could tell if bdrv_nb_sectors(bs) == 0 should be considered and error in the save_setup() context ? Thanks, C.
diff --git a/migration/block.c b/migration/block.c index 8c6ebafacc1ffe930d1d4f19d968817b14852c69..aa65ec718c2875ad9d1c40c971035f14d8086a6e 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(); @@ -404,6 +403,10 @@ static int init_blk_migration(QEMUFile *f) sectors = bdrv_nb_sectors(bs); if (sectors <= 0) { ret = sectors; + if (ret < 0) { + error_setg(errp, "Error getting length of block device %s", + bdrv_get_device_name(bs)); + } bdrv_next_cleanup(&it); goto out; } @@ -439,9 +442,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 +713,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 +721,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 future 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> --- Changes in v4: - Added an error when bdrv_nb_sectors() returns a negative value migration/block.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)