Re: [libvirt] [PATCH 1/7] qemu: Store inactive domain configuration in snapshot

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote: In order Caveat emptor - I'm not 100% familiar with all/any of the snapshot_conf code and processing, but I'll look at the patches to provide some feedback... Editorial comment... Using "newDef" has always been a bit "confusing" to say the least. I know at one point there was a comment on list in some review that we should rename it to something else to indicate what it really is, but I have forgotten what the recommendation was. Hopefully, someone else can remember! Perhaps we can "avoid" continuing the confusion with this set of changes. Even more hopefully, someone spends the time to change domain newDef ;-)! That might even fix what I'm finding confusing about these patches... Looking at existing code prior to these changes and thinking about the context of just these changes as they relate to def/newDef - I'm not sure you're capturing "everything". For instance, I see code in qemuDomainSnapshotDiskDataCollect makes specific decisions about which disks to collect, but there's no adjustment in that code with this series. There's a comment in qemuDomainRevertToSnapshot from a hunk of code just above the !REVERT_ACTIVE_ONLY from patch 2: " /* Prepare to copy the snapshot inactive xml as the config of this * domain. * * XXX Should domain snapshots track live xml rather * than inactive xml? */ " and from qemuDomainSnapshotCreateXML above where you add the newDef copy in this patch: " /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ " So what's confusing (to say the least) is - which "def" is really being used. The code at times says inactive, but IIUC newDef is generally treated as the persistent def, but that only matters if something has caused it to be created. Of course, I assume this confusion is what you're trying to fix based on the cover letter comments! If the intention of these patches is to essentially "mimic" the domain def/newDef functionality, then perhaps thinking in terms of something like virDomainObjGetPersistentDef may be helpful to "hide" which is which and when one applies over the other. (does that even make sense?) /* Steal the inactive domain def */ VIR_STEAL_PTR(def->newDom, other->def->newDom); [yes, I realize this is just stealing the code a few lines above, but the more recent desire is to use the macro] IIUC, the above hunk is designed to grab from vm->def anything that was felt to be in the domain def making it as if it was some sort of inactive definition. But since it's copying the live definition it perhaps copies portions of the live definition that have changed, such as via hotplug. That leaves a couple of questions in my mind related to whether restoration from a snapshot would/should "restore" that hotplug element especially if it doesn't apply/exist any more. The subsequent hunk essentially copies the persistent definition, so does "PARSE_INACTIVE" make sense? John BTW: I'll respond individually to other patches, but it may take a few hours as I have some errands to run today - so I'll be "in and out" - figured it was better to get this out there first though.