[libvirt PATCHv4 11/15] qemu: add code for handling virtiofsd
Peter Krempa
pkrempa at redhat.com
Wed Feb 26 08:42:04 UTC 2020
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 at redhat.com>
> ---
[...]
> 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.
More information about the libvir-list
mailing list