Re: [libvirt] [PATCH v1 3/5] qemu.conf: Introduce memory_predictable_file_names

On 10/23/2017 06:47 PM, Daniel P. Berrange wrote: > On Mon, Oct 23, 2017 at 05:43:16PM +0200, Michal Privoznik wrote: >> In some cases management application needs to allocate memory for >> qemu upfront and then just let qemu use that. Since we don't want >> to expose path for memory-backend-file anywhere in the domain >> XML, we can have a configuration knob that when enabled generated >> predictable paths. In this case: >> >> $memoryBackingDir/libvirt/qemu/$shortName/$alias >> >> where $shortName is result of virDomainObjGetShortName(). > > Looking at the current test cases, it seems that for huge pages > we currently create a directory > > $hugepagesMountPoint/libvirt/qemu/$shortName > > and for non-hugepages we just use > > $memoryBackingDir (== /var/lib/libvirt/ram) > > In both cases, we then just let QEMU create a random tempfile > name within these directories. > Correct. > So, IIUC, your proposal here is just making the non-hugepages > case work the same way the hugepages case works, except that > it is adding an extra level of $alias in the directory path. Almost. $alias is actual file, not dir.

> I don't think this extra level of nesting is really desirable, > or needed. We should just give QEMU a filename, so that it > doesn't create a random tempfile. This is achieved by simply > doing a > > mkdir($memoryBackingDir/libvirt/qemu/$shortName) > > but then giving a > > mem-path=$memoryBackingDir/libvirt/qemu/$shortName/$alias Yes. That's exactly what I'm doing. A snippet from next patch where we can see this in action: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args index 5700c3413..352819429 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-memshared.args @@ -13 +13,1 @@ QEMU_AUDIO_DRV=none \ --object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram,\ +-object memory-backend-file,id=ram-node0,mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-QEMUGuest1/ram-node0,\ And even though you're not disputing '/libvirt/qemu/' part in the path, I'll just point out for future reference that memoryBackingDir can be just any path in the system. It's just that the default is /var/lib/libvirt/qemu which makes the generated path ugly (and indeed renders the part redundant).

> QEMU will try to open that, and will get ENOENT, at which > point it to O_CREAT that file with the exact name we've > given, instead of using a tempfile name. > > >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/libvirtd_qemu.aug | 1 + >> src/qemu/qemu.conf | 5 ++ >> src/qemu/qemu_command.c | 9 +-- >> src/qemu/qemu_conf.c | 76 +++++++++++++++++++- >> src/qemu/qemu_conf.h | 10 ++- >> src/qemu/qemu_driver.c | 19 +++++ >> src/qemu/qemu_hotplug.c | 2 +- >> src/qemu/qemu_process.c | 141 ++++++++++++++++++++++++++----------- >> src/qemu/qemu_process.h | 8 +-- >> src/qemu/test_libvirtd_qemu.aug.in | 1 + >> 10 files changed, 220 insertions(+), 52 deletions(-) > > I'd expect to see current tests updated wrt the new naming scheme > for paths See next patch.

> NB, this creates potentially many levels in the dir hiearchy > > $memoryBackingDir/libvirt > $memoryBackingDir/libvirt/qemu/ > $memoryBackingDir/libvirt/qemu/$shortName Only these ^^ are dirs. > $memoryBackingDir/libvirt/qemu/$shortName/$alias This is actual file.

>> + if (qemuSecurityDomainSetPathLabel(driver->securityManager, >> + def, path) < 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Unable to label %s"), path); >> + return -1; >> + } >> + } else { >> + if (rmdir(path) < 0 && >> + errno != ENOENT) >> + VIR_WARN("Unable to remove hugepage path: %s (errno=%d)", >> + path, errno); > > This only deletes > > $memoryBackingDir/libvirt/qemu/$shortName/$alias > > so we're leaving around > > $memoryBackingDir/libvirt/qemu/$shortName Is that so? I'm fairly certain it removes $memoryBackingDir/libvirt/qemu/$shortName