[PATCH] qemu: fix excluding disk from internal inactive snapshot

[Nikolay Shirokovskiy]

ср, 16 февр. 2022 г. в 15:55, Peter Krempa <pkrempa at redhat.com>:

> On Wed, Feb 16, 2022 at 15:33:57 +0300, Nikolay Shirokovskiy wrote:
> > ср, 16 февр. 2022 г. в 11:58, Peter Krempa <pkrempa at redhat.com>:
> >
> > > On Wed, Feb 16, 2022 at 11:38:01 +0300, Nikolay Shirokovskiy wrote:
> > > > Currently taking an internal inactive snapshot with a disk excluded
> from
> > > > snapshot using snapshot="no" will fail with error "Disk device '%s'
> does
> > > > not support snapshotting".
> > >
> > > The idea for internal snapshots since we wanted them to behave the same
> > > as they do with 'savevm' is that you can't actually exclude a disk from
> > > the snapshot.
> > >
> > > Could you elaborate how you expect this to co-operate with live
> internal
> > > snapshots?
> > >
> > >
> > I have an issue with readonly usb raw disk. Making internal snapshot of
> an
> > active domain
> > is possible in this case if I exclude the disk from snapshot explicitly
> or
> > implicitly.
>
> Well, yeah, you can set it as explicitly excluded since it would be
> excluded implicitly. The other way should not be possible.
>
> > However in case of inactive domain snapshot fails with the above error.
> So
> > I guess
> > we should allow this case.
> >
> > So maybe we should check for readonly property of the disk instead? On
> > snapshot creation
> > there will be no difference as check in virDomainSnapshotAlignDisks will
> > fail for non
> > readonly disks.
>
> [...]
>
> > Yet it makes more sense to me to check snapdef disk
> > because qemuDomainSnapshotForEachQcow2Raw
> > is only to call qemu-img and the decision about the disk set is not its
> > concern. Also
> > the function is used on snapshot reverting/deletion and we want to base
> > on snapdef again and not the current state of readonly attribute on disk.
>
> Okay, so the problem isn't actually with the code in this patch, because
> virDomainSnapshotAlignDisks already ensures that the snapshot wouldn't
> be done under the same conditions as with the live snapshots.
>
> It's the commit message that is misleading because you didn't at all
> mention that the disk was readonly and thus wouldn not take part in the
> snapshot anyway.
>
> Thus the code you are changing is correctly addressing the bug, where
> the offline snapshotting function is attempting to take the snapshot of
> the readonly disk.
>
> I suggest the following commit message:
>
> 'qemuDomainSnapshotForEachQcow2Raw' doesn't properly handle the
> 'VIR_DOMAIN_SNAPSHOT_LOCATION_NONE' setting and thus doesn't skip disks
> which were excluded from the snapshot due to being read-only.
>
>
> With that commit message you can use:
>
> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
>

Actually I was not aware of all the details at that moment and just
occasionally
the patch happened to be correct. So thanx for the review!

Nikolay
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220216/7533b83b/attachment-0001.htm>