On Thu, Aug 01, 2024 at 06:04:59 -0700, Andrea Bolognani wrote: > On Thu, Aug 01, 2024 at 12:03:52PM GMT, Peter Krempa wrote: > > On Thu, Aug 01, 2024 at 02:35:59 -0700, Andrea Bolognani wrote: > > > This looks unnecessarily verbose. I kinda get the idea, but > > > considering that we're unlikely to go back and do anything about most > > > of the architectures that we're currently leaving alone, does it > > > really make sense to have all this code instead of just > > > > > > bool stripACPI = false; > > > > > > /* S390 never supported ACPI, but some users enabled it regardless in their > > > * configs for some reason. In order to fix incoming migration we'll strip > > > * ACPI from those definitions, so that user configs are fixed by updating > > > * only the destination libvirt installation */ > > > if (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X) > > > stripACPI = true; > > > > > > ? Maybe there's some motivation that I'm not seeing. > > > > > > Well, I tend to prefer the switch statement because it forces you to > > consider every code path when adding another member into the enum. In > > this case though, you're right that we won't ever want to add anything > > to the case when stripping is allowed. > > > > On the other hand it clearly differentiates the legacy arches where > > theoretically the same bug could be considered to be fixed and the > > modern arches where that stuff must never be done. > > > > Also note that yes this is more lines, but I'd consider the "verbosity" > > to be at the same level, because you then don't have to consider what > > other options are around when reading the code. And yes, all of that > > could be replaced by a comment. > > I generally prefer the switch approach as well, it just seems > overkill in this specific case. But it's perfectly valid, so feel > free to just ignore this bit of feedback. > > > > Regarding the disadvantage you mention in the commit message, how > > > about we implement a middle-ground solution instead? If we only > > > actually strip the feature when > > > > > > !(parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) > > > > > > then we still silently fix migration, but we also prevent new s390x > > > domains with ACPI (supposedly) enabled from being defined, and > > > generally limit the scope of the workaround as much as possible. > > > > I've considered this, but in case when you migrate the persistent XML > > along with the migration or use offline migration, you won't be able to > > start the VM. Yes it will force you to fix the config but it feels wrong > > to allow migration but then have a definition which no longer works. > > > > If the s390 folks consider this important for their usage and want to > > forgo the fact that we'll be stripping <acpi/> instead of them fixing > > the configs in the first place, we should do that always to prevent > > having weird intermediate states. > > > > If we want to go the route of not allowing 'defining' a new config with > > acpi enabled, we'll need another VIR_DOMAIN_DEF_PARSE_ flag asserted on > > all migration related XML parsing where the modification will be > > performed and on none of the normal code paths, but none of the existing > > flags allow that granularity. > > So offline migration sets ABI_UPDATE? I guess that makes sense to > some extent, as live migration obviously has the strictest possible > requirements, but still it feels weird that the same domain migrated > between the same two hosts could end up with slightly different ABI > depending on whether or not it's migrated while it's running. Well, I thought it did but it doesn't seem to be the case: src/qemu/qemu_driver.c=static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, src/qemu/qemu_driver.c: VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; src/qemu/qemu_driver.c=static char *qemuConnectDomainXMLToNative(virConnectPtr conn, src/qemu/qemu_driver.c: VIR_DOMAIN_DEF_PARSE_ABI_UPDATE))) src/qemu/qemu_driver.c=qemuDomainDefineXMLFlags(virConnectPtr conn, src/qemu/qemu_driver.c: VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; src/qemu/qemu_driver.c=qemuDomainAttachDeviceLiveAndConfig(virDomainObj *vm, src/qemu/qemu_driver.c: VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; Which are the only relevant code paths asserting it. > > I can tell you for certain that the presence of ABI_UPDATE is > (mis)interpreted as a witness for "this is a newly-defined domain" in > several places. I've introduced a few myself. Having a separate flag > that actually carries that meaning would probably be a very good > idea. Well, I thought this flag would be asserted in more code paths, as of the 4 above, 1 is irrelevant and 3 are safe I guess we can do this thing as well.
