Re: [PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/12/20 1:09 PM, Max Reitz wrote:
> On 12.10.20 12:16, Philippe Mathieu-Daudé wrote:
>> On 10/12/20 12:07 PM, Max Reitz wrote:
>>> On 08.10.20 19:48, Philippe Mathieu-Daudé wrote:
>>>> From: Daniel P. Berrangé <berrange@redhat.com>
>>>>
>>>> The bdrv_all_*_snapshot functions return a BlockDriverState pointer
>>>> for the invalid backend, which the callers then use to report an
>>>> error message. In some cases multiple callers are reporting the
>>>> same error message, but with slightly different text. In the future
>>>> there will be more error scenarios for some of these methods, which
>>>> will benefit from fine grained error message reporting. So it is
>>>> helpful to push error reporting down a level.
>>>>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> ---
>>>>    include/block/snapshot.h       | 14 +++----
>>>>    block/monitor/block-hmp-cmds.c |  7 ++--
>>>>    block/snapshot.c               | 77 +++++++++++++++++-----------------
>>>>    migration/savevm.c             | 37 +++++-----------
>>>>    monitor/hmp-cmds.c             |  7 +---
>>>>    replay/replay-debugging.c      |  4 +-
>>>>    tests/qemu-iotests/267.out     | 10 ++---
>>>>    7 files changed, 67 insertions(+), 89 deletions(-)
>>>
>>> Looks good overall to me, but for some reason this patch pulls in the
>>> @ok and @ret variables from the top scope of all concerned functions
>>> into the inner scopes of the BDS loops, and drops their initialization.
>>>    That’s wrong, because we only call the respective snapshotting
>>> functions on some BDSs, so the return value stays uninitialized for all
>>> other BDSs:
>>
>> Indeed, thanks for catching that.
>>
>> [...]
>>>>    int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>>>>                                 BlockDriverState *vm_state_bs,
>>>>                                 uint64_t vm_state_size,
>>>> -                             BlockDriverState **first_bad_bs)
>>>> +                             Error **errp)
>>>>    {
>>>> -    int err = 0;
>>>>        BlockDriverState *bs;
>>>>        BdrvNextIterator it;
>>>>          for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>>>>            AioContext *ctx = bdrv_get_aio_context(bs);
>>>> +        int ret;
>>>
>>> And one final time.
>>>
>>> Max
>>>
>>>>            aio_context_acquire(ctx);
>>>>            if (bs == vm_state_bs) {
>>>>                sn->vm_state_size = vm_state_size;
>>>> -            err = bdrv_snapshot_create(bs, sn);
>>>> +            ret = bdrv_snapshot_create(bs, sn);
>>>>            } else if (bdrv_all_snapshots_includes_bs(bs)) {
>>>>                sn->vm_state_size = 0;
>>>> -            err = bdrv_snapshot_create(bs, sn);
>>>> +            ret = bdrv_snapshot_create(bs, sn);
>>
>> This one is not needed.
> 
> Why not?  Is bdrv_all_snapshots_includes_bs(bs) guaranteed to be true?
> (I don’t see any a plain “else” branch, or where ret would be set
> outside of these two “if” blocks.)

Oops, I misread the 'else' branch, you are right again :)

> 
> Max
> 
>>>>            }
>>>>            aio_context_release(ctx);
>>>> -        if (err < 0) {
>>>> +        if (ret < 0) {
>>>> +            error_setg(errp, "Could not create snapshot '%s' on '%s'",
>>>> +                       sn->name, bdrv_get_device_or_node_name(bs));
>>>>                bdrv_next_cleanup(&it);
>>>> -            goto fail;
>>>> +            return -1;
>>>>            }
>>>>        }
>>>
>>
>