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

[Max Reitz]

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

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