Re: [libvirt PATCH 2/2] conf: Fix migration in some firmware autoselection scenarios

On Wed, Apr 12, 2023 at 04:52:16AM -0700, Andrea Bolognani wrote: > On Wed, Apr 12, 2023 at 12:55:51PM +0200, Ján Tomko wrote: > > On a Tuesday in 2023, Andrea Bolognani wrote: > > > static int > > > virDomainDefParseBootFirmwareOptions(virDomainDef *def, > > > - xmlXPathContextPtr ctxt) > > > + xmlXPathContextPtr ctxt, > > > + unsigned int flags) > > > { > > > g_autofree char *firmware = virXPathString("string(./os/@firmware)", ctxt); > > > g_autofree xmlNodePtr *nodes = NULL; > > > g_autofree int *features = NULL; > > > + bool abiUpdate = !!(flags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE); > > > > The flag is documented as: > > /* allow updates in post parse callback that would break ABI otherwise */ > > VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 7, > > > > and I also think that this is something that better belongs in > > post-parse. > > Okay, so the idea would be to keep picking up the firmware features > here and possibly drop them from the DomainDef during the PostParse > phase? I think that could work too. Let me give it a try. This doesn't seem to work as smoothly as I hoped it would :( The problem is that, if the kludge is performed in PostParse, the call tree ends up looking like virDomainDefPostParse() qemuDomainDefPostParse() qemuFirmwareFillDomain() <- firmware selection virDomainDefOSValidate() virDomainDefPostParseCommon() virDomainDefPostParseOs() <- migration kludge and firmware selection fails, because the configuration it sees is the original one, which doesn't pass validation. I could take virDomainDefPostParseOs() out of virDomainDefPostParseCommon() and ensure it gets called before the driver-specific PostParse callback, but to be honest I'm not convinced making the kludge even more invasive would represent an improvement.

Other ideas?