Re: [libvirt] [PATCH] qemu: save config after pivot only if domain is persistent

On Tue, Jan 05, 2016 at 11:38:07 +1100, Michael Chapman wrote: > On Mon, 4 Jan 2016, Peter Krempa wrote: >> On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote: ... >>> @@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, >>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) >>> VIR_WARN("Unable to save status on vm %s after block job", >>> vm->def->name); >>> - if (persistDisk && virDomainSaveConfig(cfg->configDir, >>> - vm->newDef) < 0) >>> + if (vm->persistent && persistDisk && >> >> I'm not quite sure how this would happen. If there isn't a persistent >> configuration, how did we get to having the persistDisk object? >> >> If this happened in some way, the problem then is that vm->newDef was >> not freed or is present in any way so we should fix the origin and not >> this symptom. > > I spent a lot of time trying to work this out myself, and although I can > see what the code is doing I don't really understand the rationale or > history behind it all. > > vm->newDef is supposed to be the "new domain definition to activate on > shutdown", according to domain_conf.h. What's confusing though is that it > is possible for this to exist even on transient domains. That is not confusing but a bug. The newDef should not exist if the domain is transient.

As a side note, all the naming of def, newDef and virDomainObjSetDefTransient is very unfortunate. Having a persistentDef and liveDef or similarly named pointers would be more clear, easier to differentiate states and would additionally allow to get rid of vm->persistent since it would be equal to vm->persistentDef being set or not. If you want to fix the undefine bug you are welcome, otherwise I'll do it in a few days. Peter