[libvirt] [PATCH 0/4] qemu: Don't leave temp mount dirs around
After yesterday's patches [1] and fixing BZ [2] with them, the comment 4 got me thinking: why leave /var/run/libvirt/$domain.pts and similar temporary directories around? We need them just for a very short time. After that we might as well remove them. 1: https://www.redhat.com/archives/libvir-list/2017-January/msg00073.html 2: https://bugzilla.redhat.com/show_bug.cgi?id=1406837 Michal Privoznik (4): qemuDomainCreateNamespace: s/unlink/rmdir/ qemuDomainGetPreservedMounts: Do not special case /dev qemuDomainCreateNamespace: move mkdir to qemuDomainBuildNamespace qemu: Drop qemuDomainDeleteNamespace src/qemu/qemu_domain.c | 126 +++++++++++++----------------------------------- src/qemu/qemu_domain.h | 3 -- src/qemu/qemu_process.c | 2 - 3 files changed, 34 insertions(+), 97 deletions(-) -- 2.11.0
If something goes wrong in this function we try a rollback. That is unlink all the directories we created earlier. For some weird reason unlink() was called instead of rmdir(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 495d86a01..f7326c73e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7467,9 +7467,9 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, cleanup: if (ret < 0) { if (devPath) - unlink(devPath); + rmdir(devPath); for (i = 0; i < ndevMountsSavePath; i++) - unlink(devMountsSavePath[i]); + rmdir(devMountsSavePath[i]); } virStringListFreeCount(devMountsSavePath, ndevMountsSavePath); VIR_FREE(devPath); -- 2.11.0
The c1140eb9e got me thinking. We don't want to special case /dev in qemuDomainGetPreservedMounts(), but in all other places in the code we special case it anyway. I mean, /var/run/libvirt/$domain.dev path is constructed separately just so that it is not constructed here. It makes only a little sense (if any at all). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 63 ++++++++++++++++++++------------------------------ 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7326c73e..54481214c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -226,18 +226,17 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr driver, return 0; } - /* Since the list is sorted and only has paths that start with /dev, the - * /dev itself can only be first. */ - if (STREQ(mounts[0], "/dev")) - VIR_DELETE_ELEMENT(mounts, 0, nmounts); - if (VIR_ALLOC_N(paths, nmounts) < 0) goto error; for (i = 0; i < nmounts; i++) { + const char *suffix = mounts[i] + strlen(DEVPREFIX); + + if (STREQ(mounts[i], "/dev")) + suffix = "dev"; + if (virAsprintf(&paths[i], "%s/%s.%s", - cfg->stateDir, vm->def->name, - mounts[i] + strlen(DEVPREFIX)) < 0) + cfg->stateDir, vm->def->name, suffix) < 0) goto error; } @@ -7344,20 +7343,32 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver, goto cleanup; } - if (virAsprintf(&devPath, "%s/%s.dev", - cfg->stateDir, vm->def->name) < 0) - goto cleanup; - if (qemuDomainGetPreservedMounts(driver, vm, &devMountsPath, &devMountsSavePath, &ndevMountsPath) < 0) goto cleanup; + for (i = 0; i < ndevMountsPath; i++) { + if (STREQ(devMountsPath[i], "/dev")) { + devPath = devMountsSavePath[i]; + break; + } + } + + if (!devPath) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to find any /dev mount")); + goto cleanup; + } + if (qemuDomainSetupDev(driver, vm, devPath) < 0) goto cleanup; /* Save some mount points because we want to share them with the host */ for (i = 0; i < ndevMountsPath; i++) { + if (devMountsSavePath[i] == devPath) + continue; + if (mount(devMountsPath[i], devMountsSavePath[i], NULL, mount_flags, NULL) < 0) { virReportSystemError(errno, @@ -7393,6 +7404,9 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver, } for (i = 0; i < ndevMountsPath; i++) { + if (devMountsSavePath[i] == devPath) + continue; + if (virFileMakePath(devMountsPath[i]) < 0) { virReportSystemError(errno, _("Cannot create %s"), devMountsPath[i]); @@ -7412,7 +7426,6 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver, ret = 0; cleanup: virObjectUnref(cfg); - VIR_FREE(devPath); virStringListFreeCount(devMountsPath, ndevMountsPath); virStringListFreeCount(devMountsSavePath, ndevMountsPath); return ret; @@ -7425,7 +7438,6 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; - char *devPath = NULL; char **devMountsSavePath = NULL; size_t ndevMountsSavePath = 0, i; @@ -7435,22 +7447,11 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, goto cleanup; } - if (virAsprintf(&devPath, "%s/%s.dev", - cfg->stateDir, vm->def->name) < 0) - goto cleanup; - if (qemuDomainGetPreservedMounts(driver, vm, NULL, &devMountsSavePath, &ndevMountsSavePath) < 0) goto cleanup; - if (virFileMakePath(devPath) < 0) { - virReportSystemError(errno, - _("Failed to create %s"), - devPath); - goto cleanup; - } - for (i = 0; i < ndevMountsSavePath; i++) { if (virFileMakePath(devMountsSavePath[i]) < 0) { virReportSystemError(errno, @@ -7466,13 +7467,10 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, ret = 0; cleanup: if (ret < 0) { - if (devPath) - rmdir(devPath); for (i = 0; i < ndevMountsSavePath; i++) rmdir(devMountsSavePath[i]); } virStringListFreeCount(devMountsSavePath, ndevMountsSavePath); - VIR_FREE(devPath); virObjectUnref(cfg); return ret; } @@ -7513,22 +7511,11 @@ qemuDomainDeleteNamespace(virQEMUDriverPtr driver, if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return; - if (virAsprintf(&devPath, "%s/%s.dev", - cfg->stateDir, vm->def->name) < 0) - goto cleanup; - if (qemuDomainGetPreservedMounts(driver, vm, NULL, &devMountsSavePath, &ndevMountsSavePath) < 0) goto cleanup; - if (rmdir(devPath) < 0) { - virReportSystemError(errno, - _("Unable to remove %s"), - devPath); - /* Bet effort. Fall through. */ - } - for (i = 0; i < ndevMountsSavePath; i++) { if (rmdir(devMountsSavePath[i]) < 0) { virReportSystemError(errno, -- 2.11.0
Again, there is no need to create /var/lib/libvirt/$domain.* directories in CreateNamespace(). It is sufficient to create them as soon as we need them which is in BuildNamespace. This way we don't leave them around for the whole lifetime of domain. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 54481214c..137d68e47 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7369,6 +7369,13 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver, if (devMountsSavePath[i] == devPath) continue; + if (virFileMakePath(devMountsSavePath[i]) < 0) { + virReportSystemError(errno, + _("Failed to create %s"), + devMountsSavePath[i]); + goto cleanup; + } + if (mount(devMountsPath[i], devMountsSavePath[i], NULL, mount_flags, NULL) < 0) { virReportSystemError(errno, @@ -7426,6 +7433,8 @@ qemuDomainBuildNamespace(virQEMUDriverPtr driver, ret = 0; cleanup: virObjectUnref(cfg); + for (i = 0; i < ndevMountsPath; i++) + rmdir(devMountsSavePath[i]); virStringListFreeCount(devMountsPath, ndevMountsPath); virStringListFreeCount(devMountsSavePath, ndevMountsPath); return ret; @@ -7438,8 +7447,6 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; - char **devMountsSavePath = NULL; - size_t ndevMountsSavePath = 0, i; if (!virBitmapIsBitSet(cfg->namespaces, QEMU_DOMAIN_NS_MOUNT) || !virQEMUDriverIsPrivileged(driver)) { @@ -7447,30 +7454,11 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver, goto cleanup; } - if (qemuDomainGetPreservedMounts(driver, vm, - NULL, &devMountsSavePath, - &ndevMountsSavePath) < 0) - goto cleanup; - - for (i = 0; i < ndevMountsSavePath; i++) { - if (virFileMakePath(devMountsSavePath[i]) < 0) { - virReportSystemError(errno, - _("Failed to create %s"), - devMountsSavePath[i]); - goto cleanup; - } - } - if (qemuDomainEnableNamespace(vm, QEMU_DOMAIN_NS_MOUNT) < 0) goto cleanup; ret = 0; cleanup: - if (ret < 0) { - for (i = 0; i < ndevMountsSavePath; i++) - rmdir(devMountsSavePath[i]); - } - virStringListFreeCount(devMountsSavePath, ndevMountsSavePath); virObjectUnref(cfg); return ret; } -- 2.11.0
After previous commits, this function is no longer needed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 33 --------------------------------- src/qemu/qemu_domain.h | 3 --- src/qemu/qemu_process.c | 2 -- 3 files changed, 38 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 137d68e47..01765dc28 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7486,39 +7486,6 @@ qemuDomainCreateNamespace(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, #endif /* !defined(__linux__) */ -void -qemuDomainDeleteNamespace(virQEMUDriverPtr driver, - virDomainObjPtr vm) -{ - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - char *devPath = NULL; - char **devMountsSavePath = NULL; - size_t ndevMountsSavePath = 0, i; - - - if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) - return; - - if (qemuDomainGetPreservedMounts(driver, vm, - NULL, &devMountsSavePath, - &ndevMountsSavePath) < 0) - goto cleanup; - - for (i = 0; i < ndevMountsSavePath; i++) { - if (rmdir(devMountsSavePath[i]) < 0) { - virReportSystemError(errno, - _("Unable to remove %s"), - devMountsSavePath[i]); - /* Bet effort. Fall through. */ - } - } - cleanup: - virObjectUnref(cfg); - virStringListFreeCount(devMountsSavePath, ndevMountsSavePath); - VIR_FREE(devPath); -} - - struct qemuDomainAttachDeviceMknodData { virQEMUDriverPtr driver; virDomainObjPtr vm; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index cce879f76..88b586972 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -806,9 +806,6 @@ int qemuDomainBuildNamespace(virQEMUDriverPtr driver, int qemuDomainCreateNamespace(virQEMUDriverPtr driver, virDomainObjPtr vm); -void qemuDomainDeleteNamespace(virQEMUDriverPtr driver, - virDomainObjPtr vm); - int qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d8593ba83..a980f5a9e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6271,8 +6271,6 @@ void qemuProcessStop(virQEMUDriverPtr driver, } } - qemuDomainDeleteNamespace(driver, vm); - vm->taint = 0; vm->pid = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); -- 2.11.0
On Fri, Jan 06, 2017 at 11:30:39AM +0100, Michal Privoznik wrote:
After yesterday's patches [1] and fixing BZ [2] with them, the comment 4 got me thinking: why leave /var/run/libvirt/$domain.pts and similar temporary directories around? We need them just for a very short time. After that we might as well remove them.
1: https://www.redhat.com/archives/libvir-list/2017-January/msg00073.html 2: https://bugzilla.redhat.com/show_bug.cgi?id=1406837
Michal Privoznik (4): qemuDomainCreateNamespace: s/unlink/rmdir/ qemuDomainGetPreservedMounts: Do not special case /dev qemuDomainCreateNamespace: move mkdir to qemuDomainBuildNamespace qemu: Drop qemuDomainDeleteNamespace
src/qemu/qemu_domain.c | 126 +++++++++++++----------------------------------- src/qemu/qemu_domain.h | 3 -- src/qemu/qemu_process.c | 2 - 3 files changed, 34 insertions(+), 97 deletions(-)
ACK series. Jan
participants (2)
-
Ján Tomko -
Michal Privoznik