Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse

On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote: > On 9/20/18 1:44 PM, Marc Hartmayer wrote: > > ...although priv->qemuCaps will be NULL in almost every case when the > > post parse callback has failed. That may change in the future. > > > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com> > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com> > --- > > src/qemu/qemu_process.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > > index 44c63c42d618..6c5a6472d8cd 100644 > > --- a/src/qemu/qemu_process.c > > +++ b/src/qemu/qemu_process.c > > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver, > The comment just above here could use a tweak for grammar ;-): > /* in case when the post parse callback failed we need to re-run it > on the > * old config prior we start the VM */ > > if (vm->def->postParseFailed) { > > VIR_DEBUG("re-running the post parse callback"); > > > - if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) > > + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0) > Searching through history of this line finds Peter's original commit in > this area - 7726d158, which seems to indicate a very specific reason for > providing a NULL capabilities value here. > I think from this patch on is something Peter has worked on a lot, so I > would prefer to defer to Peter on them since I'm sure he understands all > the various PostParse and Validate special conditions and various flags > usage better than I do. Thanks for notifying me.

In general, when parsing a "new" domain config as it's the case here, NULL should be passed in.

The qemu driver registers the 'domainPostParseDataAlloc' callback to qemuDomainPostParseDataAlloc. This callback is executed after the 'basic' post parse callback which fills in the emulator binary. Passing an explicit qemuCaps is meant only for special cases e.g. migration where we have a specific set of capabilities which need to be used rather than a new one.

This patch does not seem to make sense with the justification provided here as the post-parse callbacks fill in the proper caps here and this part clearly uses a new domain configuration, so the default behaviour should be used. Changing this would also need that we change the normal parser path to achieve the same results which is impossible as you don't have access to priv->qemuCaps prior to creating the virDomainObj object.