On Wed, Feb 26, 2020 at 09:42:04AM +0100, Peter Krempa wrote:
Explicitly CCing danpb to clarify usage of the logging daemon. On Thu, Feb 20, 2020 at 15:32:48 +0100, Ján Tomko wrote:Start virtiofsd for each <filesystem> device using it. Pre-create the socket for communication with QEMU and pass it to virtiofsd. Note that virtiofsd needs to run as root. https://bugzilla.redhat.com/show_bug.cgi?id=1694166 Introduced by QEMU commit a43efa34c7d7b628cbf1ec0fe60043e5c91043ea Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> ---[...]diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c new file mode 100644 index 0000000000..600a3644bd --- /dev/null +++ b/src/qemu/qemu_virtiofs.c @@ -0,0 +1,300 @@[...]+char * +qemuVirtioFSCreatePidFilename(virQEMUDriverConfigPtr cfg, + const virDomainDef *def, + const char *alias) +{ + g_autofree char *shortName = NULL; + g_autofree char *name = NULL; + + if (!(shortName = virDomainDefGetShortName(def))) + return NULL; + + name = g_strdup_printf("%s-%s-virtiofsd", shortName, alias); + + return virPidFileBuildPath(cfg->stateDir, name); +}Why do we want to store the pid file here?+ + +char * +qemuVirtioFSCreateSocketFilename(virDomainObjPtr vm, + const char *alias) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + return virFileBuildPath(priv->libDir, alias, "-virtiofsd.sock");Shouldn't we put it here as well? It's a sub-process of the domain itself and doesn't make much sense to treat it as the qemu pid file.
Right, libDir sounds like a better location. [...]
+ + logpath = qemuVirtioFSCreateLogFilename(cfg, vm->def, fs->info.alias); + + if (cfg->stdioLogD) { + if ((logfd = virLogManagerDomainOpenLogFile(logManager, + "qemu", + vm->def->uuid, + vm->def->name, + logpath, + 0, + NULL, NULL)) < 0)Hmm, I'm not sure now what the intented usage of uuid/domname was and there's no docs. Daniel, please review this.
Grepping for uuid in src/logging, this seems to be only used when saving and loading virtlockd's state. But virtiofsd's lifecycle is tied to the domain, so I don't think it should get its own uuid.
+ goto cleanup; + } else { + if ((logfd = open(logpath, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR)) < 0) { + virReportSystemError(errno, _("failed to create logfile %s"), + logpath); + goto cleanup; + } + if (virSetCloseExec(logfd) < 0) { + virReportSystemError(errno, _("failed to set close-on-exec flag on %s"), + logpath); + goto error; + } + } + + if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd))) + goto cleanup; + + /* so far only running as root is supported */ + virCommandSetUID(cmd, 0); + virCommandSetGID(cmd, 0);Is it necessary to special-case this for cases when we run the session connection? Or is it necessary to forbid usage of virtiofs?
Right, session users deserve a nicer error message until virtiofsd learns to run as non-root. Jano
Attachment:
signature.asc
Description: PGP signature
