4:30 a.m.
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
4:30 a.m.
New subject: [libvirt] [PATCH 1/4] qemuDomainCreateNamespace: s/unlink/rmdir/
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(a)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
4:30 a.m.
New subject: [libvirt] [PATCH 2/4] qemuDomainGetPreservedMounts: Do not special case /dev
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(a)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
4:30 a.m.
New subject: [libvirt] [PATCH 3/4] qemuDomainCreateNamespace: move mkdir to qemuDomainBuildNamespace
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(a)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
4:30 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Drop qemuDomainDeleteNamespace
After previous commits, this function is no longer needed.
Signed-off-by: Michal Privoznik <mprivozn(a)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
5:03 a.m.
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
2873
days inactive
2877
days old
5 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik