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. > +} [...] > +static int > +qemuVirtioFSOpenChardev(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + const char *socket_path) > +{ > + virDomainChrSourceDefPtr chrdev = virDomainChrSourceDefNew(NULL); > + virDomainChrDef chr = { .source = chrdev }; > + VIR_AUTOCLOSE fd = -1; > + int ret = -1; > + > + chrdev->type = VIR_DOMAIN_CHR_TYPE_UNIX; > + chrdev->data.nix.listen = true; > + chrdev->data.nix.path = g_strdup(socket_path); > + > + if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def) < 0) > + goto cleanup; > + fd = qemuOpenChrChardevUNIXSocket(chrdev); > + if (fd < 0) { > + ignore_value(qemuSecurityClearSocketLabel(driver->securityManager, vm->def)); > + goto cleanup; > + } > + if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0) > + goto cleanup; > + > + if (qemuSecuritySetChardevLabel(driver, vm, &chr) < 0) > + goto cleanup; > + > + ret = fd; > + fd = -1; > + > + cleanup: > + virObjectUnref(chrdev); > + return ret; > +} > + > +static virCommandPtr Inconsistent spacing between functions. > +qemuVirtioFSBuildCommandLine(virQEMUDriverConfigPtr cfg, > + virDomainFSDefPtr fs, > + int *fd) > +{ > + g_autoptr(virCommand) cmd = NULL; > + g_auto(virBuffer) opts = VIR_BUFFER_INITIALIZER; > + > + if (!(cmd = virCommandNew(fs->binary))) > + return NULL; > + > + virCommandAddArgFormat(cmd, "--fd=%d", *fd); > + virCommandPassFD(cmd, *fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); > + *fd = -1; > + > + virCommandAddArg(cmd, "-o"); > + virBufferAsprintf(&opts, "source=%s,", fs->src->path); Since the path is an argument of -o along with others you must escape commas in the path. > + if (fs->cache) > + virBufferAsprintf(&opts, "cache=%s,", virDomainFSCacheModeTypeToString(fs->cache)); > + > + if (fs->xattr == VIR_TRISTATE_SWITCH_ON) > + virBufferAddLit(&opts, "xattr,"); > + else if (fs->xattr == VIR_TRISTATE_SWITCH_OFF) > + virBufferAddLit(&opts, "no_xattr,"); > + > + if (fs->flock == VIR_TRISTATE_SWITCH_ON) > + virBufferAddLit(&opts, "flock,"); > + else if (fs->flock == VIR_TRISTATE_SWITCH_OFF) > + virBufferAddLit(&opts, "no_flock,"); > + > + if (fs->posix_lock == VIR_TRISTATE_SWITCH_ON) > + virBufferAddLit(&opts, "posix_lock,"); > + else if (fs->posix_lock == VIR_TRISTATE_SWITCH_OFF) > + virBufferAddLit(&opts, "no_posix_lock,"); > + > + virBufferTrim(&opts, ","); > + > + virCommandAddArgBuffer(cmd, &opts); > + if (cfg->virtiofsdDebug) > + virCommandAddArg(cmd, "-d"); > + > + return g_steal_pointer(&cmd); > +} > + > +int > +qemuVirtioFSStart(virLogManagerPtr logManager, > + virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainFSDefPtr fs) > +{ > + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); > + g_autoptr(virCommand) cmd = NULL; > + g_autofree char *socket_path = NULL; > + g_autofree char *pidfile = NULL; > + g_autofree char *logpath = NULL; > + pid_t pid = (pid_t) -1; > + VIR_AUTOCLOSE fd = -1; > + VIR_AUTOCLOSE logfd = -1; > + int ret = -1; > + int rc; > + > + if (!virFileExists(fs->src->path)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("the virtiofs export directory '%s' does not exist"), > + fs->src->path); > + return -1; > + } > + > + if (!(pidfile = qemuVirtioFSCreatePidFilename(cfg, vm->def, fs->info.alias))) > + goto cleanup; > + > + if (!(socket_path = qemuVirtioFSCreateSocketFilename(vm, fs->info.alias))) > + goto cleanup; > + > + if ((fd = qemuVirtioFSOpenChardev(driver, vm, socket_path)) < 0) > + goto cleanup; > + > + 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? > + virCommandSetPidFile(cmd, pidfile); > + virCommandSetOutputFD(cmd, &logfd); > + virCommandSetErrorFD(cmd, &logfd); > + virCommandNonblockingFDs(cmd); > + virCommandDaemonize(cmd); > + > + if (qemuExtDeviceLogCommand(driver, vm, cmd, "virtiofsd") < 0) > + goto cleanup; > + The rest looks good to me.
