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