<meta http-equiv="Content-Type" content="text/html; charset=utf-8"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:large"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">ср, 16 февр. 2022 г. в 15:55, Peter Krempa <<a href="mailto:pkrempa@redhat.com">pkrempa@redhat.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Feb 16, 2022 at 15:33:57 +0300, Nikolay Shirokovskiy wrote:<br>
> ср, 16 февр. 2022 г. в 11:58, Peter Krempa <<a href="mailto:pkrempa@redhat.com" target="_blank">pkrempa@redhat.com</a>>:<br>
> <br>
> > On Wed, Feb 16, 2022 at 11:38:01 +0300, Nikolay Shirokovskiy wrote:<br>
> > > Currently taking an internal inactive snapshot with a disk excluded from<br>
> > > snapshot using snapshot="no" will fail with error "Disk device '%s' does<br>
> > > not support snapshotting".<br>
> ><br>
> > The idea for internal snapshots since we wanted them to behave the same<br>
> > as they do with 'savevm' is that you can't actually exclude a disk from<br>
> > the snapshot.<br>
> ><br>
> > Could you elaborate how you expect this to co-operate with live internal<br>
> > snapshots?<br>
> ><br>
> ><br>
> I have an issue with readonly usb raw disk. Making internal snapshot of an<br>
> active domain<br>
> is possible in this case if I exclude the disk from snapshot explicitly or<br>
> implicitly.<br>
<br>
Well, yeah, you can set it as explicitly excluded since it would be<br>
excluded implicitly. The other way should not be possible.<br>
<br>
> However in case of inactive domain snapshot fails with the above error. So<br>
> I guess<br>
> we should allow this case.<br>
> <br>
> So maybe we should check for readonly property of the disk instead? On<br>
> snapshot creation<br>
> there will be no difference as check in virDomainSnapshotAlignDisks will<br>
> fail for non<br>
> readonly disks.<br>
<br>
[...]<br>
<br>
> Yet it makes more sense to me to check snapdef disk<br>
> because qemuDomainSnapshotForEachQcow2Raw<br>
> is only to call qemu-img and the decision about the disk set is not its<br>
> concern. Also<br>
> the function is used on snapshot reverting/deletion and we want to base<br>
> on snapdef again and not the current state of readonly attribute on disk.<br>
<br>
Okay, so the problem isn't actually with the code in this patch, because<br>
virDomainSnapshotAlignDisks already ensures that the snapshot wouldn't<br>
be done under the same conditions as with the live snapshots.<br>
<br>
It's the commit message that is misleading because you didn't at all<br>
mention that the disk was readonly and thus wouldn not take part in the<br>
snapshot anyway.<br>
<br>
Thus the code you are changing is correctly addressing the bug, where<br>
the offline snapshotting function is attempting to take the snapshot of<br>
the readonly disk.<br>
<br>
I suggest the following commit message:<br>
<br>
'qemuDomainSnapshotForEachQcow2Raw' doesn't properly handle the<br>
'VIR_DOMAIN_SNAPSHOT_LOCATION_NONE' setting and thus doesn't skip disks<br>
which were excluded from the snapshot due to being read-only.<br>
<br>
<br>
With that commit message you can use:<br>
<br>
Reviewed-by: Peter Krempa <<a href="mailto:pkrempa@redhat.com" target="_blank">pkrempa@redhat.com</a>><br></blockquote><div><br></div><div class="gmail_default" style="font-size:large">Actually I was not aware of all the details at that moment and just occasionally</div><div class="gmail_default" style="font-size:large">the patch happened to be correct. So thanx for the review!</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">Nikolay </div></div></div>