Re: [PATCH v2 1/4] qemu.conf changes to support multiple memory backend

On Wed, Jun 05, 2024 at 04:37:35PM -0400, mgalaxy(a)akamai.com wrote: > From: Michael Galaxy <mgalaxy(a)akamai.com&gt; > > +                               _("Domain requesting configuration > for %1$lu NUMA nodes, but memory backing directory only has (%2$lu) > directory paths available. Either reduce this to one directory or > provide more paths to use."), > +                               numa_node_count, > +                               cfg->nb_memoryBackingDirs); > +                return -1; > +            } > + > +            path_index = virBitmapNextSetBit(numaBitmap, -1); What if the single pinning is to the third host node, but there are only two memoryBackingDirs supplied in the config?

> { > +    virQEMUDriver *driver = priv->driver; >     g_autofree char *path = NULL; > +    g_autoptr(virQEMUDriverConfig) cfg = > virQEMUDriverGetConfig(driver); > +    size_t i; > > -    if (qemuGetMemoryBackingPath(driver, vm->def, mem->info.alias, > &path) < 0) > -        return -1; > +    for (i = 0; i < cfg->nb_memoryBackingDirs; i++) { > +        virDomainXMLPrivateDataCallbacks *privateData = > (virDomainXMLPrivateDataCallbacks *) priv; > +        if (qemuGetMemoryBackingPath(def, privateData, i, > mem->info.alias, &path) < 0) > +            return -1; > I wonder why are the paths not saved in the status XML of the running domain.  Since they can be changed during the VM's lifetime (while restarting libvirt) it could cause issues during clean-up.  Sure, unlink() will set errno to ENOENT, but the proper paths will not be cleaned up.  It'd also make this part of the code easier and safer to use from the callers.  But that's pre-existing.

> -    if (unlink(path) < 0 && > -        errno != ENOENT) { > -        virReportSystemError(errno, _("Unable to remove %1$s"), path); > -        return -1; > +        if (unlink(path) < 0 && errno != ENOENT) { > +            virReportSystemError(errno, _("Unable to remove %1$s"), > path); > +            return -1; > +        } >     } > >     return 0; > @@ -7268,7 +7274,7 @@ qemuProcessPrepareHost(virQEMUDriver *driver, >     if (qemuProcessPrepareHostBackendChardev(vm) < 0) >         return -1; > > -    if (qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, true) < 0) > +    if (qemuProcessBuildDestroyMemoryPaths(vm, NULL, true) < 0) >         return -1; > >     /* Ensure no historical cgroup for this VM is lying around bogus > @@ -8482,7 +8488,7 @@ void qemuProcessStop(virQEMUDriver *driver, >         goto endjob; >     } > > -    qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false); > +    qemuProcessBuildDestroyMemoryPaths(vm, NULL, false); > >     if (!!g_atomic_int_dec_and_test(&driver->nactive) && > driver->inhibitCallback) >         driver->inhibitCallback(false, driver->inhibitOpaque); > diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h > index c1ea949215..eebd0a4d1b 100644 > --- a/src/qemu/qemu_process.h > +++ b/src/qemu/qemu_process.h > @@ -38,13 +38,12 @@ int qemuProcessStopCPUs(virQEMUDriver *driver, >                         virDomainPausedReason reason, >                         virDomainAsyncJob asyncJob); > > -int qemuProcessBuildDestroyMemoryPaths(virQEMUDriver *driver, > -                                       virDomainObj *vm, > +int qemuProcessBuildDestroyMemoryPaths(virDomainObj *vm, >                                        virDomainMemoryDef *mem, >                                        bool build); > > -int qemuProcessDestroyMemoryBackingPath(virQEMUDriver *driver, > -                                        virDomainObj *vm, > +int qemuProcessDestroyMemoryBackingPath(virDomainDef *def, > +                                        qemuDomainObjPrivate *priv, >                                         virDomainMemoryDef *mem); > > void qemuProcessReconnectAll(virQEMUDriver *driver); > -- > 2.34.1 > This patch changes the configuration, so it should also adjust the documentation and since configuration is used everywhere it should also add a few unit tests with the changed configuration and few edge cases, just so we can be sure it really works the way it is supposed to.