Re: [libvirt] [PATCH v4 2/2] snapshot: Store both config and live XML in the snapshot domain

On Thu, Aug 29, 2019 at 17:55:43 -0300, Maxiwell S. Garcia wrote: OK, I can live with this since dom/inactiveDom are direct equivalents to domain/inactiveDomain elements in the snapshot XML, ... ... ... ...but the code here and the comment above should be changed a bit. You can't point both config and inactiveConfig to the same virDomainDef structure (see below). I believe config/inactiveConfig should be used strictly for active/inactive domain definition (thus config could be NULL for inactive snapshots) and we should copy the definition twice for active snapshots without inactiveDom: + if (snap->def->inactiveDom) { + inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps, + driver->xmlopt, priv->qemuCaps, true); + if (!inactiveConfig) + goto endjob; + } else { + /* Inactive domain definition is missing: + * - either this is an old active snapshot and we need to copy the + * active definition as an inactive one + * - or this is an inactive snapshot which means config contains the + * inactive definition. + */ + if (snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || + snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) { + inactiveConfig = virDomainDefCopy(snap->def->dom, caps, + driver->xmlopt, priv->qemuCaps, true); + if (!inactiveConfig) + goto endjob; + } else { + VIR_STEAL_PTR(inactiveConfig, config); + } + } If both domain and inactiveDomain elements were present in the snapshot XML, you'd leak config here. I realized that the config can be leaked even with the current code if qemuDomainRevertToSnapshot fails so I sent a patch fixing that: https://www.redhat.com/archives/libvir-list/2019-September/msg00382.html Once the memory leak is fixed, this hunk needs a trivial change to fit the new code. In case inactiveDomain was not present in the snapshot XML, both inactiveConfig and config would contain the same pointer. Thus you'd assign a single domain definition twice, which would result in funny things such as memory corruption and double free once one of them is freed for some reason. This is fixed by using the modified code for filling config and inactiveConfig (see above) and following the changes I made to avoid leaking memory (msg00382.html). This should be ok since we are reverting to a snapshot of a shutoff domain and thus the snapshot XML will contain only one domain definition. However, this hunk needs a trivial change to fit the new code after the memory leak fix is pushed. With the suggested changes Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt; I'll send the patch with all modifications (which I will push once the memory leak fix is acked) as a reply to this email for completeness.