[libvirt] [PATCH 8/9] wip: start virtiofsd
Daniel P. Berrangé
berrange at redhat.com
Fri Nov 22 17:07:30 UTC 2019
On Fri, Nov 01, 2019 at 01:16:06PM +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.
> ---
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_extdevice.c | 191 ++++++++++++++++++++++++++++++++++++++
> tests/qemuxml2argvtest.c | 6 ++
> 3 files changed, 198 insertions(+)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 54a7e7c52f..78f88a0c2f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -817,6 +817,7 @@ struct _virDomainFSDef {
> unsigned long long space_soft_limit; /* in bytes */
> bool symlinksResolved;
> virDomainVirtioOptionsPtr virtio;
> + char *vhost_user_fs_path; /* TODO put this in private data */
> };
IIUC, this is a socket rather than a file, so I'd call it
"vhostuser_fs_sock" personally.
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index 463f76c21a..cb44ca325f 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -20,6 +20,7 @@
>
> #include <config.h>
>
> +#include "qemu_command.h"
> #include "qemu_extdevice.h"
> #include "qemu_vhost_user_gpu.h"
> #include "qemu_domain.h"
> @@ -149,6 +150,178 @@ qemuExtDevicesCleanupHost(virQEMUDriverPtr driver,
> qemuExtTPMCleanupHost(def);
> }
>
> +static char *
> +qemuExtVirtioFSCreatePidFilename(virQEMUDriverConfigPtr cfg,
> + const virDomainDef *def,
> + const char *alias)
> +{
> + g_autofree char *shortName = NULL;
> + g_autofree char *name = NULL;
> +
> + if (!(shortName = virDomainDefGetShortName(def)) ||
> + virAsprintf(&name, "%s-%s-virtiofsd", shortName, alias) < 0)
> + return NULL;
g_strdup_printf so we can assume this always succeeds
> +
> + return virPidFileBuildPath(cfg->stateDir, name);
> +}
likewise we can assume this aborts on oom
> +
> +
> +static char *
> +qemuExtVirtioFSCreateSocketFilename(virQEMUDriverConfigPtr cfg,
> + const virDomainDef *def,
> + const char *alias)
> +{
> + g_autofree char *shortName = NULL;
> + g_autofree char *name = NULL;
> +
> + if (!(shortName = virDomainDefGetShortName(def)) ||
> + virAsprintf(&name, "%s-%s-virtiofsd", shortName, alias) < 0)
> + return NULL;
> +
> + return virFileBuildPath(cfg->stateDir, name, ".sock");
> +}
Same comments
> +static int
> +qemuExtVirtioFSdStart(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainFSDefPtr fs)
> +{
> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> + g_autoptr(virCommand) cmd = NULL;
> + g_autofree char *pidfile = NULL;
> + char errbuf[1024] = { 0 };
> + virDomainChrSourceDefPtr chrdev = virDomainChrSourceDefNew(NULL);
> + pid_t pid = (pid_t) -1;
> + VIR_AUTOCLOSE errfd = -1;
> + VIR_AUTOCLOSE fd = -1;
> + int exitstatus = 0;
> + int cmdret = 0;
> + int ret = -1;
> + int rc;
> +
> + chrdev->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> + chrdev->data.nix.listen = true;
> +
> + if (!(pidfile = qemuExtVirtioFSCreatePidFilename(cfg, vm->def, fs->info.alias)))
> + goto cleanup;
> +
> + if (!(chrdev->data.nix.path = qemuExtVirtioFSCreateSocketFilename(cfg, vm->def, fs->info.alias)))
> + goto cleanup;
No ned to check for failure of these two.
> +
> + if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def) < 0)
> + goto cleanup;
> + fd = qemuOpenChrChardevUNIXSocket(chrdev);
> + if (fd < 0)
> + goto cleanup;
> + if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0)
> + goto cleanup;
> +
> + /* TODO: do not call this here */
> + virDomainChrDef chr = { .source = chrdev };
> + if (qemuSecuritySetChardevLabel(driver, vm, &chr) < 0)
> + goto cleanup;
Indeed, needs to be in the security manager code.
> +
> + /* TODO: configure path */
> + if (!(cmd = virCommandNew("/usr/libexec/virtiofsd")))
> + goto cleanup;
In $QEMU/docs/interop/vhost-user.json there's a specificiation for
how we're expected to locate the virtiofsd binary based on metadata
files. Same approach we're using for locating firmware files
basically.
> +
> + virCommandSetPidFile(cmd, pidfile);
> + virCommandSetErrorFD(cmd, &errfd);
> + virCommandDaemonize(cmd);
> +
> + virCommandAddArg(cmd, "--syslog");
I feel like maybe we should be using a logfile as we do for
QEMU, via virtlogd ?
> + virCommandAddArgFormat(cmd, "--fd=%d", fd);
> + virCommandPassFD(cmd, fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> + fd = -1;
> +
> + virCommandAddArg(cmd, "-o");
> + /* TODO: validate path? */
> + virCommandAddArgFormat(cmd, "source=%s", fs->src->path);
> + virCommandAddArg(cmd, "-d");
IIRC, it was decided intended that we'd have a dbus interface to
virtiofsd, one of whose jobs would be a way to turn on / off
logging, similar to our own virt-admin facility.
Could perhaps be useful to have debug on from startip, but
a qemu.conf setting is probably sufficient for this.
> +
> + if (qemuExtDeviceLogCommand(driver, vm, cmd, "virtiofsd") < 0)
> + goto cleanup;
> +
> + cmdret = virCommandRun(cmd, &exitstatus);
> +
> + if (cmdret < 0 || exitstatus != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not start 'virtiofsd'. exitstatus: %d"), exitstatus);
> + goto error;
> + }
> +
> + rc = virPidFileReadPath(pidfile, &pid);
> + if (rc < 0) {
> + virReportSystemError(-rc,
> + _("Unable to read virtiofsd pidfile '%s'"),
> + pidfile);
> + goto error;
> + }
> +
> + if (virProcessKill(pid, 0) != 0) {
> + if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) {
> + virReportSystemError(errno, "%s",
> + _("virtiofsd died unexpectedly"));
> + } else {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("virtiofsd died and reported: %s"), errbuf);
> + }
> + goto error;
> + }
I feel like we should be putting it into the cgroup for the VM
as a psuedo "emulator thread".
We'll also need to make sure that virtiofsd gets given an SELinux
policy, with sVirt MCS tag associated with it.
This raises an interesting question though, because from libvirt's
POV we're not supposed to know what binary we're actually invoking.
We're supposed to follow the vhost-user.json spec to find an
binary, allowing the user to drop in arbitrary impls.
Each impl though may have differeing SELinux requirements,
so we need to figure out what SElinux domain type to apply.
I think this means vhost-user.json needs to specify the
desired SELinux domain for each vhost user binary.
> + fs->vhost_user_fs_path = g_steal_pointer(&chrdev->data.nix.path);
> + ret = 0;
> +
> + cleanup:
> + if (chrdev->data.nix.path)
> + unlink(chrdev->data.nix.path);
> + virObjectUnref(chrdev);
> + return ret;
> +
> + error:
> + if (pid != -1)
> + virProcessKillPainfully(pid, true);
> + if (pidfile)
> + unlink(pidfile);
> + goto cleanup;
> +}
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
More information about the libvir-list
mailing list