On Fri, Jan 24, 2020 at 11:27:18 +0100, Michal Privoznik wrote: > Since v5.6.0-48-g270583ed98 we try to cache domain capabilities, > i.e. store filled virDomainCaps in a hash table in virQEMUCaps > for future use. However, there's a race condition in the way it's > implemented. We use virQEMUCapsGetDomainCapsCache() to obtain the > pointer to the hash table, then we search the hash table for > cached data and if none is found the domcaps is constructed and > put into the table. Problem is that this is all done without any > locking, so if there are two threads trying to do the same, one > will succeed and the other will fail inserting the data into the > table. > > Also, the API looks a bit fishy - obtaining pointer to the hash > table is dangerous. > > The solution is to use a mutex that guards the whole operation > with the hash table. Then, the API can be changes to return > virDomainCapsPtr directly. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1791790 > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > --- > src/qemu/qemu_capabilities.c | 118 +++++++++++++++++++++++++++++++++-- > src/qemu/qemu_capabilities.h | 14 ++++- > src/qemu/qemu_conf.c | 69 +++++--------------- > 3 files changed, 141 insertions(+), 60 deletions(-) [...] > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index 8040f8ab3a..e0cb8be675 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -1337,31 +1337,20 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver, > } > > > -struct virQEMUDriverSearchDomcapsData { > - const char *path; > - const char *machine; > - virArch arch; > - virDomainVirtType virttype; > -}; > - > - > static int > -virQEMUDriverSearchDomcaps(const void *payload, > - const void *name G_GNUC_UNUSED, > - const void *opaque) > +virQEMUDriverGetDomainCapabilitiesFiller(virDomainCapsPtr domCaps, > + virQEMUCapsPtr qemuCaps, > + void *opaque) > { > - virDomainCapsPtr domCaps = (virDomainCapsPtr) payload; > - struct virQEMUDriverSearchDomcapsData *data = (struct virQEMUDriverSearchDomcapsData *) opaque; > - > - if (STREQ_NULLABLE(data->path, domCaps->path) && > - STREQ_NULLABLE(data->machine, domCaps->machine) && > - data->arch == domCaps->arch && > - data->virttype == domCaps->virttype) > - return 1; > + virQEMUDriverPtr driver = opaque; > + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); > > - return 0; > + return virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch, domCaps, > + driver->privileged, > + cfg->firmwares, cfg->nfirmwares); This is weird. You have +virQEMUDriverGetDomainCapabilitiesFiller in qemu_conf.c which internally uses virQEMUCapsFillDomainCaps exported by qemu_capabilities.c and pass it to virQEMUCapsGetDomainCapsCache which is defined in qemu_capabilities.c. Can't we just get rid of the callback? The rest looks good.
