Re: [libvirt PATCHv4 11/15] qemu: add code for handling virtiofsd

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(a)redhat.com&gt; > --- [...] > 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.

> + > + 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.

> + 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?