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é <berra...@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é <berra...@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; >>> } >>> } >> >