[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