On Fri, Sep 19, 2025 at 5:44 PM Peter Krempa <[email protected]> wrote:
> On Fri, Sep 19, 2025 at 17:09:08 +0800, [email protected] wrote: > > From: Hyman Huang <[email protected]> > > > > Add a thorough check in the virDomainObjSave path to make sure that > > private data in the status XML file always exists for the running VM > > so that we won't lose them after restart the service. > > --- > > src/conf/domain_conf.c | 21 ++++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > > index 281846dfbe..74af08e584 100644 > > --- a/src/conf/domain_conf.c > > +++ b/src/conf/domain_conf.c > > @@ -29542,9 +29542,24 @@ virDomainObjFormat(virDomainObj *obj, > > obj->deprecations[i]); > > } > > > > - if (xmlopt->privateData.format && > > - xmlopt->privateData.format(&buf, obj) < 0) > > - return NULL; > > + if (xmlopt->privateData.format) { > > + if (xmlopt->privateData.format(&buf, obj) < 0) > > + return NULL; > > + } else { > > + /* > > + * Add a thorough check in the virDomainObjSave path to make > > + * sure that private data in the status XML file always exists > > + * for the running VM so that we won't lose them after restart > > + * the service. > > + */ > > + if (virDomainObjIsActive(obj) && > > + (obj->def->virtType == VIR_DOMAIN_VIRT_KVM)) { > > + VIR_WARN("Do not omit private data when formatting the" > > + " status XML for a %s VM", > > + virDomainStateTypeToString(state)); > > + return NULL; > > + } > > + } > > So IIUC (based on the description in the cover letter) you're saying > that there's a possibility that on the shutdown of the qemu driver > something unregisters the private data formatting functions and thus > we'd overwrite the status XML with a XML missing private data, right? > > In such case this is not the right fix. It just covers up the that fact > and also prints unactionable warnings caused by internal bug. > > The proper fix is that the private data formatters must not be > unregistered if we have internal state. > > Can you elaborate when that happens? This seems like a race condition in > the shutdown code. > > The xmlopt driver contains privateData that implements the parse/format interface: virQEMUDriverCreateXMLConf virDomainXMLOptionNew xmlopt->privateData = &virQEMUDriverPrivateDataCallbacks; The shutdown code frees the xmlopt field in qemu_driver, thereby releasing the parse/format interface: qemuStateCleanup virObjectUnref(qemu_driver->xmlopt); Meanwhile, the migration thread may call the virDomainObjSave function during the execution of qemuMigrationSrcConfirmPhase or qemuMigrationDstFinish: virDomainObjSave virDomainObjFormat if (xmlopt->privateData.format && xmlopt->privateData.format(&buf, obj) < 0) return NULL; As shown above, the shutdown code and the migration thread race to access the privateData format interface. -- Best regards
