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

[Michael Chapman]

On Wed, 6 Jan 2016, Peter Krempa wrote:
> 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.

Well, it was certainly confusing to me, but that's probably because I was
assuming the code was correct and it was just my understanding of it that
was wrong. :-)

[...]

> 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

I would be more comfortable if you were to fix up the undefine bug. You've
clearly got a better understanding of how it's all supposed to work.

However, I will send through a patch fixing the possible NULL dereference
should virStorageSourceCopy fail.

- Michael