Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
On Mon, May 20, 2024 at 14:48:47 +, Efim Shevrin via Devel wrote: > Hello, > > > If vmdisk is NULL, shouldn't this function (qemuSnapshotDeleteValidate()) > > return an error? > > I think this qemuSnapshotDeleteValidate should not return an error. > > It seems to me that when vmdisk is NULL, this does not invalidate > the snapshot itself, but indicates that the config has changed since > the snapshot was done. And if the VM config has changed, this adds evidence > that the snapshot should be deleted, > because the snapshot does not reflect the real vm config. > > Since we do not have an analogue of the --force option for deleting a > snapshot, in the case when qemuSnapshotDeleteValidate returns > an error when vmdisk is NULL, we will never delete a snapshot which has > invalid disk. Snapshot deletion does have something that can be considered force and that is the '--metadata' option that removes just the snapshot definition (metadata) and doesn't touch the disk images. > > Similarly, disk can be NULL too > Thank you for the comment regarding the disk variable. I`ve reworked patch. > > When creating a snapshot of a VM with multiple hard disks, > the snapshot takes into account the presence of all disks > in the system. If, over time, one of the disks is deleted, > the snapshot will continue to store knowledge of the deleted disk. > This results in the fact that at the moment of deleting the snapshot, > at the validation stage, a disk from the snapshot will be searched which > is not in the VM configuration. As a result, vmdisk variable will > be equal to NULL. Dereferencing a null pointer at the time of calling > virStorageSourceIsSameLocation(vmdisk->src, disk->src) > will result in SIGSEGV. Crashing is obviously not okay ... > Also, the disk variable can also be equal to NULL and this > requires to check that disk != NULL before calling the > virStorageSourceIsSameLocation function to avoid SIGSEGV. .. but going ahead with the snapshot deletion isn't always okay either. The disk isn't referenced by the VM so the disk state can't be merged, while the state would be merged for any other disk. When reverting back to a previous snapshot, which is still referencing the older state of the disk which was removed from the VM, the VM would see that the image state of disks that were present at deletion would contain the merged state, but only a partial state for the disk which was later removed.
Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
Hello,
> If vmdisk is NULL, shouldn't this function (qemuSnapshotDeleteValidate())
> return an error?
I think this qemuSnapshotDeleteValidate should not return an error.
It seems to me that when vmdisk is NULL, this does not invalidate
the snapshot itself, but indicates that the config has changed since
the snapshot was done. And if the VM config has changed, this adds evidence
that the snapshot should be deleted,
because the snapshot does not reflect the real vm config.
Since we do not have an analogue of the --force option for deleting a snapshot,
in the case when qemuSnapshotDeleteValidate returns
an error when vmdisk is NULL, we will never delete a snapshot which has invalid
disk.
> Similarly, disk can be NULL too
Thank you for the comment regarding the disk variable. I`ve reworked patch.
When creating a snapshot of a VM with multiple hard disks,
the snapshot takes into account the presence of all disks
in the system. If, over time, one of the disks is deleted,
the snapshot will continue to store knowledge of the deleted disk.
This results in the fact that at the moment of deleting the snapshot,
at the validation stage, a disk from the snapshot will be searched which
is not in the VM configuration. As a result, vmdisk variable will
be equal to NULL. Dereferencing a null pointer at the time of calling
virStorageSourceIsSameLocation(vmdisk->src, disk->src)
will result in SIGSEGV.
Also, the disk variable can also be equal to NULL and this
requires to check that disk != NULL before calling the
virStorageSourceIsSameLocation function to avoid SIGSEGV.
Signed-off-by: Fima Shevrin
---
src/qemu/qemu_snapshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 09ec959f10..ba13c43ff3 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -3806,7 +3806,7 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
-if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
+if (vmdisk != NULL && disk != NULL &&
!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("disk image '%1$s' for internal snapshot
'%2$s' is not the same as disk image currently used by VM"),
snapDisk->name, snap->def->name);
--
2.39.3
From: Michal Prívozník
Sent: Monday, May 20, 2024 15:52
To: Efim Shevrin ; devel@lists.libvirt.org
Cc: d...@openvz.org
Subject: Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the
VM config
[You don't often get email from mpriv...@redhat.com. Learn why this is
important at https://aka.ms/LearnAboutSenderIdentification ]
On 4/29/24 14:43, Fima Shevrin via Devel wrote:
> When creating a snapshot of a VM with multiple hard disks,
> the snapshot takes into account the presence of all disks
> in the system. If, over time, one of the disks is deleted,
> the snapshot will continue to store knowledge of the deleted disk.
> This results in the fact that at the moment of deleting the snapshot,
> at the validation stage, a disk from the snapshot will be searched which
> is not in the VM configuration. As a result, vmdisk variable will
> be equal to NULL. Dereferencing a null pointer at the time of calling
> virStorageSourceIsSameLocation(vmdisk->src, disk->src) will result in SIGSEGV.
>
> Signed-off-by: Fima Shevrin
> ---
> src/qemu/qemu_snapshot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 09ec959f10..bf93cd485e 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -3806,7 +3806,7 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
> vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
>
> -if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
> +if (vmdisk != NULL &&
> !virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
If vmdisk is NULL, shouldn't this function
(qemuSnapshotDeleteValidate()) return an error?
Similarly, disk can be NULL too.
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("disk image '%1$s' for internal snapshot
> '%2$s' is not the same as disk image currently used by VM"),
> snapDisk->name, snap->def->name);
Michal
Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
On 4/29/24 14:43, Fima Shevrin via Devel wrote:
> When creating a snapshot of a VM with multiple hard disks,
> the snapshot takes into account the presence of all disks
> in the system. If, over time, one of the disks is deleted,
> the snapshot will continue to store knowledge of the deleted disk.
> This results in the fact that at the moment of deleting the snapshot,
> at the validation stage, a disk from the snapshot will be searched which
> is not in the VM configuration. As a result, vmdisk variable will
> be equal to NULL. Dereferencing a null pointer at the time of calling
> virStorageSourceIsSameLocation(vmdisk->src, disk->src) will result in SIGSEGV.
>
> Signed-off-by: Fima Shevrin
> ---
> src/qemu/qemu_snapshot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 09ec959f10..bf93cd485e 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -3806,7 +3806,7 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
> vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
>
> -if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
> +if (vmdisk != NULL &&
> !virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
If vmdisk is NULL, shouldn't this function
(qemuSnapshotDeleteValidate()) return an error?
Similarly, disk can be NULL too.
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("disk image '%1$s' for internal snapshot
> '%2$s' is not the same as disk image currently used by VM"),
> snapDisk->name, snap->def->name);
Michal
Re: [PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
On 4/29/24 14:43, Fima Shevrin wrote:
When creating a snapshot of a VM with multiple hard disks,
the snapshot takes into account the presence of all disks
in the system. If, over time, one of the disks is deleted,
the snapshot will continue to store knowledge of the deleted disk.
This results in the fact that at the moment of deleting the snapshot,
at the validation stage, a disk from the snapshot will be searched which
is not in the VM configuration. As a result, vmdisk variable will
be equal to NULL. Dereferencing a null pointer at the time of calling
virStorageSourceIsSameLocation(vmdisk->src, disk->src) will result in SIGSEGV.
Signed-off-by: Fima Shevrin
---
src/qemu/qemu_snapshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 09ec959f10..bf93cd485e 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -3806,7 +3806,7 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
-if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
+if (vmdisk != NULL && !virStorageSourceIsSameLocation(vmdisk->src,
disk->src)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("disk image '%1$s' for internal snapshot '%2$s'
is not the same as disk image currently used by VM"),
snapDisk->name, snap->def->name);
ping
[PATCH] Сheck snapshot disk is not NULL when searching it in the VM config
When creating a snapshot of a VM with multiple hard disks,
the snapshot takes into account the presence of all disks
in the system. If, over time, one of the disks is deleted,
the snapshot will continue to store knowledge of the deleted disk.
This results in the fact that at the moment of deleting the snapshot,
at the validation stage, a disk from the snapshot will be searched which
is not in the VM configuration. As a result, vmdisk variable will
be equal to NULL. Dereferencing a null pointer at the time of calling
virStorageSourceIsSameLocation(vmdisk->src, disk->src) will result in SIGSEGV.
Signed-off-by: Fima Shevrin
---
src/qemu/qemu_snapshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 09ec959f10..bf93cd485e 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -3806,7 +3806,7 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name);
-if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) {
+if (vmdisk != NULL && !virStorageSourceIsSameLocation(vmdisk->src,
disk->src)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("disk image '%1$s' for internal snapshot
'%2$s' is not the same as disk image currently used by VM"),
snapDisk->name, snap->def->name);
--
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
