Re: [PATCH] block: Fix bdrv_activate() not to fail without medium

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 12.03.2025 um 15:37 hat Markus Armbruster geschrieben:
>> bdrv_activate() returns failure without setting an error when
>> !bs->drv.  This is suspicious.  Turns out it used to succeed then,
>> until commit 5416645fcf82 changed it to return -ENOMEDIUM.
>> 
>> Return zero instead.
>> 
>> Fixes: 5416645fcf82 (block: return error-code from bdrv_invalidate_cache)
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> The commit message sounds more theoretical. Did you find this only by
> code inspection? Do we know what the effect on real-world cases is, so
> we could add a sentence about it to the commit message? Maybe we could
> even have a qemu-iotests case to show the effect?
>
> I absolutely agree that returning -ENOMEDIUM while not setting errp is
> wrong. But without an example of what is affected, it's not obvious to
> me which part of it needs to be fixed.

Code inspection.

Here's my somewhat extended rationale for my fix.

The code of interest used to be in bdrv_invalidate_cache().  Have a look
at commit 5a8a30db477:

    commit 5a8a30db4771675480829d7d3bf35a138e9c35f1
    Author: Kevin Wolf <kwolf@redhat.com>
    Date:   Wed Mar 12 15:59:16 2014 +0100

        block: Add error handling to bdrv_invalidate_cache()

        If it returns an error, the migrated VM will not be started, but qemu
        exits with an error message.

        Signed-off-by: Kevin Wolf <kwolf@redhat.com>
        Reviewed-by: Juan Quintela <quintela@redhat.com>
        Reviewed-by: Eric Blake <eblake@redhat.com>
        Reviewed-by: Benoit Canet <benoit@irqsave.net>

    [...]
    diff --git a/include/block/block.h b/include/block/block.h
    index bd34d14109..1ed55d839a 100644
    --- a/include/block/block.h
    +++ b/include/block/block.h
    @@ -329,8 +329,8 @@ BlockDriverAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
             BlockDriverCompletionFunc *cb, void *opaque);

     /* Invalidate any cached metadata used by image formats */
    -void bdrv_invalidate_cache(BlockDriverState *bs);
    -void bdrv_invalidate_cache_all(void);
    +void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
    +void bdrv_invalidate_cache_all(Error **errp);

     void bdrv_clear_incoming_migration_all(void);

    diff --git a/include/block/block_int.h b/include/block/block_int.h
    index 4fc5ea8a65..cd5bc7308a 100644
    --- a/include/block/block_int.h
    +++ b/include/block/block_int.h
    @@ -153,7 +153,7 @@ struct BlockDriver {
         /*
          * Invalidate any cached meta-data.
          */
    -    void (*bdrv_invalidate_cache)(BlockDriverState *bs);
    +    void (*bdrv_invalidate_cache)(BlockDriverState *bs, Error **errp);

         /*
          * Flushes all data that was already written to the OS all the way down to
    diff --git a/block.c b/block.c
    index 53f5b44fbb..acb70fde3d 100644
    --- a/block.c
    +++ b/block.c
    @@ -4781,27 +4781,43 @@ flush_parent:
         return bdrv_co_flush(bs->file);
     }

    -void bdrv_invalidate_cache(BlockDriverState *bs)
    +void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
     {
    +    Error *local_err = NULL;
    +    int ret;
    +
         if (!bs->drv)  {
             return;
         }

         if (bs->drv->bdrv_invalidate_cache) {
    -        bs->drv->bdrv_invalidate_cache(bs);
    +        bs->drv->bdrv_invalidate_cache(bs, &local_err);
         } else if (bs->file) {
    -        bdrv_invalidate_cache(bs->file);
    +        bdrv_invalidate_cache(bs->file, &local_err);
    +    }
    +    if (local_err) {
    +        error_propagate(errp, local_err);
    +        return;
         }

    -    refresh_total_sectors(bs, bs->total_sectors);
    +    ret = refresh_total_sectors(bs, bs->total_sectors);
    +    if (ret < 0) {
    +        error_setg_errno(errp, -ret, "Could not refresh total sector count");
    +        return;
    +    }
     }

    -void bdrv_invalidate_cache_all(void)
    +void bdrv_invalidate_cache_all(Error **errp)
     {
         BlockDriverState *bs;
    +    Error *local_err = NULL;

         QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
    -        bdrv_invalidate_cache(bs);
    +        bdrv_invalidate_cache(bs, &local_err);
    +        if (local_err) {
    +            error_propagate(errp, local_err);
    +            return;
    +        }
         }
     }