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

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

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

-- 
Peter Xu

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.

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>