Re: [libvirt] new NULL-dereference in qemu_driver.c

On 04/28/2010 11:43 AM, Jim Meyering wrote: > Chris Lalancette wrote: >> On 04/27/2010 04:40 PM, Jim Meyering wrote: >>> Daniel P. Berrange wrote: >>> >>>> On Tue, Apr 27, 2010 at 06:45:16PM +0200, Jim Meyering wrote: >>>>> I ran clang on the very latest and it spotted this problem: >>>>> >From qemu_driver.c, around line 11100, >>>>> >>>>> else { >>>>> /* qemu is a little funny with running guests and the restoration >>>>> * of snapshots. If the snapshot was taken online, >>>>> * then after a "loadvm" monitor command, the VM is set running >>>>> * again. If the snapshot was taken offline, then after a "loadvm" >>>>> * monitor command the VM is left paused. Unpausing it leads to >>>>> * the memory state *before* the loadvm with the disk *after* the >>>>> * loadvm, which obviously is bound to corrupt something. >>>>> * Therefore we destroy the domain and set it to "off" in this case. >>>>> */ >>>>> >>>>> if (virDomainObjIsActive(vm)) { >>>>> qemudShutdownVMDaemon(driver, vm); >>>>> event = virDomainEventNewFromObj(vm, >>>>> VIR_DOMAIN_EVENT_STOPPED, >>>>> VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); >>>>> if (!vm->persistent) { >>>>> if (qemuDomainObjEndJob(vm) > 0) >>>>> virDomainRemoveInactive(&driver->domains, vm); >>>>> vm = NULL; >>>> >>>> This needs to add 'goto endjob' or possibly 'goto cleanup' >>> >>> No point in endjob, since it does nothing when vm == NULL. >>> >>> Here's a tentative patch for that and another, similar problem >>> (haven't even compiled it or run it through clang, but have to run). >>> Will follow up tomorrow. >> >> Yeah, this looks reasonable and is what I was going to submit. It >> would be good to give a test first, though. > > Can any of you easily test it? > I can't right now. Yep, this works fine with transient domains and snapshotting.