On Wed, Jun 22, 2016 at 03:00:47PM -0400, Laine Stump wrote: This looks fragile and complicated, and I've already managed to break it with the XML you provided: $ virsh attach-device f24 cont --live Device attached successfully $ virsh attach-device f24 cont --live --config error: Failed to attach device from cont error: operation failed: target scsi:1 already exists AFAIK the reason we create a deep copy instead of parsing it again is our generation of MAC addresses in the parser: commit 1e0534a770208be6b848c961785db20467deb2fc qemu: Don't parse device twice in attach/detach So the question is: how should we treat the combination of --live and --config? Try to make the persistent device as close as the live one? Or try to make it match the persistent config (while the live one would match the live config?) Either way, it would be nicer to get the device definition stable before we even try to add it to the persistent definition. Also, calling virDomainDefPostParse after device coldplug is strange, we should be adding a device that does not need ajdustments. Jan

On 06/23/2016 08:26 AM, Ján Tomko wrote: Yeah, I noticed that too just before I posted, and am still thinking about how to solve it, but I still think the situation is one step better with this patch in place (although it does have a similar effect on --live + --config attachment of PCI devices - changing from one improper behavior to another. Sigh.) This all comes down to a more general problem that I offhandledly mentioned in an email with Tomasz Flendrich (the GSoC student working on "device address abstraction") - other than the live state starting off as a copy of the config, we make no attempt to maintain consistency of device attributes between the two, and could easily end up with conflicting things between the two. Normally this is mostly transparent to the user, but could cause serious complaints from guest OSes when they are later restarted and all their nice cozy devices previously added with "--live --config" are suddenly moved (this may not make sense now, but there are some further examples below). But a "deep copy" is just formatting the object and then parsing the resulting XML again. Well, that's a bit misleading. It *is* parsing twice; it's just that after the patch, the 2nd parse is done on the result of formatting the result of the first parse rather than parsing the original xml again. Because the MAC address is auto-generated as a part of the parsing (rather than during post-parse), we're able to get a MAC address consistent between live and persistent without calling virDomainDefPostParse(). Anything set in the post-parse doesn't have this same happy existence though. (side-note - when I made the patch to allow auto-generating the index, I originally put it directly in the parser rather than post-parse, but this was (rightfully) NACKed by Cole: https://www.redhat.com/archives/libvir-list/2016-May/msg01065.html Doing it the original way would have avoided the "already exists" error, but not the more general problem of diverging/conflicting live state and persistent config.) Not just that, but what should we do with --live by itself? Should we allow someone to hotplug a device --live with attributes that would have failed if given to --config (i.e. if they specify a PCI address that is currently unused in the live domain, but is in use in the config)? I could see a valid use case for this - if the device was added with --live and the user later wanted to make that device permanent - but allowing it could also lead to undesired/unexpected device address changes. And what about the current bad behavior when you do this? virsh attach interface f24 --type network --source default --live virsh attach interface f24 --type network --source default --live --config Without my patch, the 2nd device ends up with a different PCI address in the live and persistent object. With my patch, you end up with a similar error to above (complaining about attempting to use a PCI address that's already in use). So which evil is worse? Refusing to give a device different info for live vs. config (arguably not *as bad* for a PCI address vs. a MAC address or controller index, but still very undesirable)? Or silently allowing the evil, and damn the consequences? The latter would just cause some extra bookkeeping in the guest for a PCI address (the next time you start it, the guest would think the original device had disappeared and been replaced with another at a different address, with varying levels of whinging), but if you change the index of a SCSI controller, you could end up with disks plugged into the wrong controller (hmm, I guess that's not a horrible thing either, unless the device name in the guest is based on the controller's position in the devices, and that device name is used in the OS config somewhere... Yuck - better to make sure it doesn't change.) I think we should make the two identical. For that to be true 100% of the time, we need to consider both the live and persistent config any time we're adding a device to either (or maybe just when we're adding it to both? I need to think or a minute, and I still haven't made my coffee). (BTW, we also should consider what happens when you add a scsi disk with --live --config, attaching it to a controller that was added with just --live. I guess the controller should be automatically added to the config, but "dunno, haven't tried".) The problem is that all of the stuff that "stabilizes" the device definition is done in the post-parse callback, and that requires that the device already be inserted into the domain definition. Again, we don't have the information necessary to adjust it until we at least have the domain def available (and all the post-parse callback infrastructure assumes/requires that the device already be inserted into the domain). But if we don't do the *entire* virDomainDefPostParse() then we are in danger of missing something during a hotplug. Maybe we need to rethink how we can do the post-parse stuff so that: 1) both the persistent config and (optionally) live state are supplied to the device callbacks (with assurances that the live state is ignored if it's NULL) 2) address and index assignment aren't done with a single call in the domain callback that assigns everything, but instead are done one-by-one in the device post-parse callback, assuring that newly assigned addresses aren't in use in either the persistent config or (if present) the live state. (I think the best way to do this may actually be to resurrect the idea of the pciaddrs "cache" rather than killing it (as is currently being discussed)). And now I find myself all out of words, but without a solution or sufficient motivation to look for one. Hopefully something will come up out of the conversation.