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. 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. -- Andrea Bolognani / Red Hat / Virtualization
