Thank you.
I will check it out.and resend in a new email
On Wed, Nov 27, 2024 at 5:49 PM Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
Typo in shortlog (cransh); use:
qemuSnapshotDeleteValidate: Fix crash when disk is not found in VM definition
On Thu, Nov 21, 2024 at 23:36:14 +0800, jungle man wrote:
> qemuDomainDiskByName() can return a NULL pointer on failure, but this
> returned value in qemuSnapshotDeleteValidate is not checked.
> It will make libvirtd crash.
Couple problems:
1) your patch got corrupted when sending
2) your patch is missing the certification that it is in compliance
with the Developer Certificate of Origin (missing sign-off):
https://www.libvirt.org/hacking.html#developer-certificate-of-origin
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 5b3aadcbf0..52312b4a7b 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -4235,8 +4235,11 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
> virDomainDiskDef *vmdisk = NULL;
> virDomainDiskDef *disk = NULL;
>
> - vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name);
> - disk = qemuDomainDiskByName(snapdef->parent.dom,
> snapDisk->name);
> + if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name)))
> + return -1;
> +
> + if (!(disk = qemuDomainDiskByName(snapdef->parent.dom,
> snapDisk->name)))
> + return -1;
3) While these functions do report their own error and we generallu use
that, I think in this case it'll be confusing as it will not tell the
user in which config the disk is missing. I think we need specific
errors in this case. e.g.:
if (!(vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name))) {
virReportError(VIR_ERR_OPERATION_FAILED,
_("disk '%1$s' referenced by snapshot '%2$s' not found in the current definition"),
snapDisk->name, snap->def->name);
return -1;
}
and for the other error something like:
_("disk '%1$s' referenced by snapshot '%2$s' not found in the VM definition of the deleted snapshot"),
