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

["Cédric Le Goater" <clg@redhat.com>]

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