2:42 a.m.
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>
---
[...]
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.