Thank you. I will check it out.and resend in a new email
On Wed, Nov 27, 2024 at 5:49 PM Peter Krempa <[email protected]> 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"), > >
