Message ID | 20180205151835.20812-2-mreitz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: > When invoking drive-mirror in absolute-paths mode, the target's backing > BDS is assigned to it in mirror_exit(). The current logic only does so > if the target does not have that backing BDS already; but it actually > cannot have a backing BDS at all (the BDS is opened with O_NO_BACKING in > qmp_drive_mirror()), so just assert that and assign the new backing BDS > unconditionally. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> Introducing new assumptions that the graph stays unmodified between the start of a block job and its completion feels a bit adventurous when all of the blockdev work is moving towards making things more flexible. If we really do want to enforce this, I guess you finally found a use case for BLK_PERM_GRAPH_MOD. Kevin
On 2018-02-22 13:27, Kevin Wolf wrote: > Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: >> When invoking drive-mirror in absolute-paths mode, the target's backing >> BDS is assigned to it in mirror_exit(). The current logic only does so >> if the target does not have that backing BDS already; but it actually >> cannot have a backing BDS at all (the BDS is opened with O_NO_BACKING in >> qmp_drive_mirror()), so just assert that and assign the new backing BDS >> unconditionally. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> > > Introducing new assumptions that the graph stays unmodified between the > start of a block job and its completion feels a bit adventurous when all > of the blockdev work is moving towards making things more flexible. But as the commit message hints at, MIRROR_SOURCE_BACKING_CHAIN is used only by drive-mirror. How flexible do you intend that to be? Yes, technically you can specify a node name for the newly created image and then go wild while the block job is still running. But then something inside me feels like you should see your VM crashing for that... OTOH, a crash is always bad. I can't make it crash in the current state because the mirror job creates a BB and blockdev-snapshot thus refuses to attach a backing file to the target. But I guess I'll drop this patch anyway (unless I find it was important for some other patch in this series, which I hope it wasn't...). On a totally unrelated note, we need some protocol for hurting the user if they do something that deserves a bit of pain. Crashing is always bad and our fault, so it needs to be something else. Removing $HOME? Marking all attached qcow2 images corrupt even though they aren't so you have to run a qemu-img check? I like that one. Max
diff --git a/block/mirror.c b/block/mirror.c index c9badc1203..45e35c909f 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -523,12 +523,12 @@ static void mirror_exit(BlockJob *job, void *opaque) &error_abort); if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { BlockDriverState *backing = s->is_none_mode ? src : s->base; - if (backing_bs(target_bs) != backing) { - bdrv_set_backing_hd(target_bs, backing, &local_err); - if (local_err) { - error_report_err(local_err); - data->ret = -EPERM; - } + + assert(!target_bs->backing); + bdrv_set_backing_hd(target_bs, backing, &local_err); + if (local_err) { + error_report_err(local_err); + data->ret = -EPERM; } }