Re: [PATCH V2] qemu: pre-create the dbus directory in qemuStateInitialize
On a Wednesday in 2020, Bihong Yu wrote: On 2020/7/22 13:36, Pino Toscano wrote: On Wednesday, 22 July 2020 04:09:24 CEST Bihong Yu wrote: +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { +virReportSystemError(errno, _("Failed to create dbus state dir %s"), + cfg->dbusStateDir); Minor notes on the message: - spell "D-Bus" correctly - no need to abbreviate "directory" - quote the path placeholder so I suggest something like: "Failed to create the D-Bus state directory '%s'" Sorry, the error message is written with reference to other contexts of qemuStateInitialize(), such as: if (virFileMakePath(cfg->memoryBackingDir) < 0) { virReportSystemError(errno, _("Failed to create memory backing dir %s"), cfg->memoryBackingDir); goto error; } if (virFileMakePath(cfg->slirpStateDir) < 0) { virReportSystemError(errno, _("Failed to create slirp state dir %s"), cfg->slirpStateDir); goto error; } +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { +virReportSystemError(errno, _("Failed to create dbus state dir %s"), + cfg->dbusStateDir); +goto error; +} So I don't think that's a good suggestion. If you still insist your suggestion, I will rewrite it. They are good suggestions but I'm afraid they're out of scope of this patch. Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH V2] qemu: pre-create the dbus directory in qemuStateInitialize
On 2020/7/22 13:36, Pino Toscano wrote: > On Wednesday, 22 July 2020 04:09:24 CEST Bihong Yu wrote: >> +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, >> + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { >> +virReportSystemError(errno, _("Failed to create dbus state dir %s"), >> + cfg->dbusStateDir); > > Minor notes on the message: > - spell "D-Bus" correctly > - no need to abbreviate "directory" > - quote the path placeholder > so I suggest something like: > "Failed to create the D-Bus state directory '%s'" Sorry, the error message is written with reference to other contexts of qemuStateInitialize(), such as: if (virFileMakePath(cfg->memoryBackingDir) < 0) { virReportSystemError(errno, _("Failed to create memory backing dir %s"), cfg->memoryBackingDir); goto error; } if (virFileMakePath(cfg->slirpStateDir) < 0) { virReportSystemError(errno, _("Failed to create slirp state dir %s"), cfg->slirpStateDir); goto error; } +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { +virReportSystemError(errno, _("Failed to create dbus state dir %s"), + cfg->dbusStateDir); +goto error; +} So I don't think that's a good suggestion. If you still insist your suggestion, I will rewrite it.
Re: [PATCH V2] qemu: pre-create the dbus directory in qemuStateInitialize
On Wednesday, 22 July 2020 04:09:24 CEST Bihong Yu wrote: > +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, > + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { > +virReportSystemError(errno, _("Failed to create dbus state dir %s"), > + cfg->dbusStateDir); Minor notes on the message: - spell "D-Bus" correctly - no need to abbreviate "directory" - quote the path placeholder so I suggest something like: "Failed to create the D-Bus state directory '%s'" (Can't comment on the rest of the changes, sorry.) -- Pino Toscano signature.asc Description: This is a digitally signed message part.
[PATCH V2] qemu: pre-create the dbus directory in qemuStateInitialize
>From 165abdd77c7db83ebf98232b80d6b988471185c0 Mon Sep 17 00:00:00 2001 From: Bihong Yu Date: Tue, 14 Jul 2020 15:44:05 +0800 Subject: [PATCH] qemu: pre-create the dbus directory in qemuStateInitialize There are races condiction to make '/run/libvirt/qemu/dbus' directory in virDirCreateNoFork() while concurrent start VMs, and get "failed to create directory '/run/libvirt/qemu/dbus': File exists" error message. pre-create the dbus directory in qemuStateInitialize. Signed-off-by:Bihong Yu --- src/qemu/qemu_dbus.c| 10 -- src/qemu/qemu_dbus.h| 2 -- src/qemu/qemu_driver.c | 7 +++ src/qemu/qemu_process.c | 3 --- 4 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index 51f6c94..8104287 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -33,16 +33,6 @@ VIR_LOG_INIT("qemu.dbus"); -int -qemuDBusPrepareHost(virQEMUDriverPtr driver) -{ -g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - -return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, -VIR_DIR_CREATE_ALLOW_EXIST); -} - - static char * qemuDBusCreatePidFilename(virQEMUDriverConfigPtr cfg, const char *shortName) diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h index 474eb10..3c2145a 100644 --- a/src/qemu/qemu_dbus.h +++ b/src/qemu/qemu_dbus.h @@ -21,8 +21,6 @@ #include "qemu_conf.h" #include "qemu_domain.h" -int qemuDBusPrepareHost(virQEMUDriverPtr driver); - char *qemuDBusGetAddress(virQEMUDriverPtr driver, virDomainObjPtr vm); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8e81c30..53980d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -743,6 +743,13 @@ qemuStateInitialize(bool privileged, goto error; } +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) { +virReportSystemError(errno, _("Failed to create dbus state dir %s"), + cfg->dbusStateDir); +goto error; +} + if ((qemu_driver->lockFD = virPidFileAcquire(cfg->stateDir, "driver", false, getpid())) < 0) goto error; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e8b15ee..1006f41 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6466,9 +6466,6 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); -if (qemuDBusPrepareHost(driver) < 0) -return -1; - if (qemuPrepareNVRAM(cfg, vm) < 0) return -1; -- 1.8.3.1