[PATCH 0/4] qemu: Open chardev logfile on behalf of QEMU
See 4/4 for explanation. Michal Prívozník (4): virDomainDefGetSecurityLabelDef: Fix const correctness qemuDomainOpenFile: Take virDomainDef instead of virDomainObj qemuDomainOpenFile: Take @cfg instead of driver qemu: Open chardev logfile on behalf of QEMU src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 68 +++++++++++++++++++++++++++++---------- src/qemu/qemu_domain.c | 13 ++++---- src/qemu/qemu_domain.h | 4 +-- src/qemu/qemu_driver.c | 4 +-- src/qemu/qemu_saveimage.c | 5 +-- 7 files changed, 66 insertions(+), 32 deletions(-) -- 2.31.1
The function doesn't write to domain definition really so make @def argument as const. This allows us to call it from functions where the domain definition is already const. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3415e28b95..14abd527b6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -29483,7 +29483,7 @@ virDomainDeviceDefCopy(virDomainDeviceDef *src, virSecurityLabelDef * -virDomainDefGetSecurityLabelDef(virDomainDef *def, const char *model) +virDomainDefGetSecurityLabelDef(const virDomainDef *def, const char *model) { size_t i; virSecurityLabelDef *seclabel = NULL; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9f32bcf9cf..984939cc69 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3776,7 +3776,7 @@ virDomainObjGetState(virDomainObj *obj, int *reason) ATTRIBUTE_NONNULL(1); virSecurityLabelDef * -virDomainDefGetSecurityLabelDef(virDomainDef *def, const char *model); +virDomainDefGetSecurityLabelDef(const virDomainDef *def, const char *model); virSecurityDeviceLabelDef * virDomainChrSourceDefGetSecurityLabelDef(virDomainChrSourceDef *def, -- 2.31.1
The function doesn't really need domain object, but domain definition from which it takes seclabels. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 8 ++++---- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_saveimage.c | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6f8c93ea0c..5a88e82856 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11503,7 +11503,7 @@ virQEMUFileOpenAs(uid_t fallback_uid, /** * qemuDomainOpenFile: * @driver: driver object - * @vm: domain object + * @def: domain definition * @path: path to file to open * @oflags: flags for opening/creation of the file * @needUnlink: set to true if file was created by this function @@ -11518,7 +11518,7 @@ virQEMUFileOpenAs(uid_t fallback_uid, **/ int qemuDomainOpenFile(virQEMUDriver *driver, - virDomainObj *vm, + const virDomainDef *def, const char *path, int oflags, bool *needUnlink) @@ -11530,8 +11530,8 @@ qemuDomainOpenFile(virQEMUDriver *driver, virSecurityLabelDef *seclabel; /* TODO: Take imagelabel into account? */ - if (vm && - (seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL && + if (def && + (seclabel = virDomainDefGetSecurityLabelDef(def, "dac")) != NULL && seclabel->label != NULL && (virParseOwnershipIds(seclabel->label, &user, &group) < 0)) return -EINVAL; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index acf6ca5ab6..63f657fa49 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1046,7 +1046,7 @@ int virQEMUFileOpenAs(uid_t fallback_uid, int qemuDomainOpenFile(virQEMUDriver *driver, - virDomainObj *vm, + const virDomainDef *def, const char *path, int oflags, bool *needUnlink); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7d76dd00f..d432c69dae 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10820,7 +10820,7 @@ qemuDomainStorageOpenStat(virQEMUDriver *driver, if (skipInaccessible && !virFileExists(src->path)) return 0; - if ((*ret_fd = qemuDomainOpenFile(driver, vm, src->path, O_RDONLY, + if ((*ret_fd = qemuDomainOpenFile(driver, vm->def, src->path, O_RDONLY, NULL)) < 0) return -1; diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index b4af80f942..f93454c761 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -313,7 +313,7 @@ qemuSaveImageCreate(virQEMUDriver *driver, if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0) goto cleanup; - if ((fd = qemuDomainOpenFile(driver, vm, path, O_WRONLY, NULL)) < 0 || + if ((fd = qemuDomainOpenFile(driver, vm->def, path, O_WRONLY, NULL)) < 0 || virQEMUSaveDataFinish(data, &fd, path) < 0) goto cleanup; -- 2.31.1
Again, we don't need full driver, just its config. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 5 ++--- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_saveimage.c | 5 +++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5a88e82856..2aa346744d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11502,7 +11502,7 @@ virQEMUFileOpenAs(uid_t fallback_uid, /** * qemuDomainOpenFile: - * @driver: driver object + * @cfg: driver config object * @def: domain definition * @path: path to file to open * @oflags: flags for opening/creation of the file @@ -11517,13 +11517,12 @@ virQEMUFileOpenAs(uid_t fallback_uid, * qemuDomainStorageFileInit and storage driver APIs if possible. **/ int -qemuDomainOpenFile(virQEMUDriver *driver, +qemuDomainOpenFile(virQEMUDriverConfig *cfg, const virDomainDef *def, const char *path, int oflags, bool *needUnlink) { - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); uid_t user = cfg->user; gid_t group = cfg->group; bool dynamicOwnership = cfg->dynamicOwnership; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 63f657fa49..d470dc3822 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1045,7 +1045,7 @@ int virQEMUFileOpenAs(uid_t fallback_uid, bool *needUnlink); int -qemuDomainOpenFile(virQEMUDriver *driver, +qemuDomainOpenFile(virQEMUDriverConfig *cfg, const virDomainDef *def, const char *path, int oflags, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d432c69dae..ed3af5a619 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10808,7 +10808,7 @@ qemuDomainMemoryPeek(virDomainPtr dom, * reported) or -1 otherwise (errors are reported). */ static int -qemuDomainStorageOpenStat(virQEMUDriver *driver, +qemuDomainStorageOpenStat(virQEMUDriver *driver G_GNUC_UNUSED, virQEMUDriverConfig *cfg, virDomainObj *vm, virStorageSource *src, @@ -10820,7 +10820,7 @@ qemuDomainStorageOpenStat(virQEMUDriver *driver, if (skipInaccessible && !virFileExists(src->path)) return 0; - if ((*ret_fd = qemuDomainOpenFile(driver, vm->def, src->path, O_RDONLY, + if ((*ret_fd = qemuDomainOpenFile(cfg, vm->def, src->path, O_RDONLY, NULL)) < 0) return -1; diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index f93454c761..e14e2987f1 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -313,7 +313,7 @@ qemuSaveImageCreate(virQEMUDriver *driver, if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0) goto cleanup; - if ((fd = qemuDomainOpenFile(driver, vm->def, path, O_WRONLY, NULL)) < 0 || + if ((fd = qemuDomainOpenFile(cfg, vm->def, path, O_WRONLY, NULL)) < 0 || virQEMUSaveDataFinish(data, &fd, path) < 0) goto cleanup; @@ -440,6 +440,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, bool open_write, bool unlink_corrupt) { + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOCLOSE fd = -1; int ret = -1; g_autoptr(virQEMUSaveData) data = NULL; @@ -459,7 +460,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, oflags |= directFlag; } - if ((fd = qemuDomainOpenFile(driver, NULL, path, oflags, NULL)) < 0) + if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0) return -1; if (bypass_cache && -- 2.31.1
If the QEMU driver is configured to use the old "file" stdio handler (meaning virtlogd is out of the picture) and a chardev has a log file configured we rely on QEMU being able to create the file itself. This may not be always possible (e.g. if the logfile is set to a directory that QEMU process can't reach). In such case we should create the file and just pass its FD to QEMU. We could do that unconditionally and just either pass FD from virtlogd or the one we opened, because we bumped QEMU version and are now requiring new enough QEMU. However, I'm keeping the old style where logfile is appended on the cmd line for the tests sake. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1989457 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- I don't like the typecast in qemuSecuritySetImageFDLabel() call, but fixing that turned out to be not trivial, so I left it as is. src/qemu/qemu_command.c | 68 ++++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4381ea7d8b..3aca2bb177 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4702,30 +4702,63 @@ qemuBuildSCSIHostdevDevStr(const virDomainDef *def, static int qemuBuildChrChardevFileStr(virLogManager *logManager, - virCommand *cmd, + virSecurityManager *secManager, + virQEMUDriverConfig *cfg, + virQEMUCaps *qemuCaps, const virDomainDef *def, + virCommand *cmd, virBuffer *buf, const char *filearg, const char *fileval, const char *appendarg, int appendval) { - if (logManager) { + /* Technically, to pass an FD via /dev/fdset we don't need + * any capability check because X_QEMU_CAPS_ADD_FD is already + * assumed. But keeping the old style is still handy when + * building a standalone command line (e.g. for tests). */ + if (logManager || + virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) { g_autofree char *fdset = NULL; - int flags = 0; int logfd; size_t idx; - if (appendval == VIR_TRISTATE_SWITCH_ABSENT || - appendval == VIR_TRISTATE_SWITCH_OFF) - flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; - - if ((logfd = virLogManagerDomainOpenLogFile(logManager, - "qemu", - def->uuid, - def->name, - fileval, - flags, - NULL, NULL)) < 0) - return -1; + if (logManager) { + int flags = 0; + + if (appendval == VIR_TRISTATE_SWITCH_ABSENT || + appendval == VIR_TRISTATE_SWITCH_OFF) + flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; + + if ((logfd = virLogManagerDomainOpenLogFile(logManager, + "qemu", + def->uuid, + def->name, + fileval, + flags, + NULL, NULL)) < 0) + return -1; + } else { + int oflags = O_CREAT | O_WRONLY; + + switch (appendval) { + case VIR_TRISTATE_SWITCH_ABSENT: + case VIR_TRISTATE_SWITCH_OFF: + oflags |= O_TRUNC; + break; + case VIR_TRISTATE_SWITCH_ON: + oflags |= O_APPEND; + break; + case VIR_TRISTATE_SWITCH_LAST: + break; + } + + if ((logfd = qemuDomainOpenFile(cfg, def, fileval, oflags, NULL)) < 0) + return -1; + + if (qemuSecuritySetImageFDLabel(secManager, (virDomainDef*)def, logfd) < 0) { + VIR_FORCE_CLOSE(logfd); + return -1; + } + } virCommandPassFDIndex(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); fdset = qemuBuildFDSet(logfd, idx); @@ -4868,7 +4901,7 @@ qemuBuildChrChardevStr(virLogManager *logManager, if (qemuBuildChrChardevFileStr(cdevflags & QEMU_BUILD_CHARDEV_FILE_LOGD ? logManager : NULL, - cmd, def, &buf, + secManager, cfg, qemuCaps, def, cmd, &buf, "path", dev->data.file.path, "append", dev->data.file.append) < 0) return NULL; @@ -5004,7 +5037,8 @@ qemuBuildChrChardevStr(virLogManager *logManager, } if (dev->logfile) { - if (qemuBuildChrChardevFileStr(logManager, cmd, def, &buf, + if (qemuBuildChrChardevFileStr(logManager, secManager, cfg, + qemuCaps, def, cmd, &buf, "logfile", dev->logfile, "logappend", dev->logappend) < 0) return NULL; -- 2.31.1
On Fri, Aug 06, 2021 at 05:22:34PM +0200, Michal Privoznik wrote:
See 4/4 for explanation.
Michal Prívozník (4): virDomainDefGetSecurityLabelDef: Fix const correctness qemuDomainOpenFile: Take virDomainDef instead of virDomainObj qemuDomainOpenFile: Take @cfg instead of driver qemu: Open chardev logfile on behalf of QEMU
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On a %A in %Y, Michal Privoznik wrote:
See 4/4 for explanation.
Michal Prívozník (4): virDomainDefGetSecurityLabelDef: Fix const correctness qemuDomainOpenFile: Take virDomainDef instead of virDomainObj qemuDomainOpenFile: Take @cfg instead of driver qemu: Open chardev logfile on behalf of QEMU
src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 68 +++++++++++++++++++++++++++++---------- src/qemu/qemu_domain.c | 13 ++++---- src/qemu/qemu_domain.h | 4 +-- src/qemu/qemu_driver.c | 4 +-- src/qemu/qemu_saveimage.c | 5 +-- 7 files changed, 66 insertions(+), 32 deletions(-)
Tested-by: Ján Tomko <jtomko@redhat.com>
participants (3)
-
Jano Tomko -
Michal Privoznik -
Pavel Hrdina