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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list