On Thu, Jan 23, 2020 at 18:46:09 +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 TBD, pull request here: > https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05389.html > > Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> > --- > po/POTFILES.in | 1 + > src/qemu/Makefile.inc.am | 2 + > src/qemu/qemu_domain.c | 3 + > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_extdevice.c | 19 +++ > src/qemu/qemu_virtiofs.c | 241 ++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_virtiofs.h | 37 ++++++ > tests/qemuxml2argvtest.c | 6 + > 8 files changed, 310 insertions(+), 1 deletion(-) > create mode 100644 src/qemu/qemu_virtiofs.c > create mode 100644 src/qemu/qemu_virtiofs.h [...] > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 4db0075b89..19b3eb0fab 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -1442,6 +1442,9 @@ qemuDomainFSPrivateNew(void) > static void > qemuDomainFSPrivateDispose(void *obj G_GNUC_UNUSED) 'obj' is no longer unused. > { > + qemuDomainFSPrivatePtr priv = obj; > + > + g_free(priv->vhostuser_fs_sock); > } > [...] > diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c > index 463f76c21a..1f4b605eb3 100644 > --- a/src/qemu/qemu_extdevice.c > +++ b/src/qemu/qemu_extdevice.c [...] > @@ -184,6 +186,16 @@ qemuExtDevicesStart(virQEMUDriverPtr driver, > return -1; > } Not required but I'd be grateful if you get rid of the pointless 'ret' variable in this function. Context in this patch made me question how that actually works. > > + for (i = 0; i < def->nfss; i++) { > + virDomainFSDefPtr fs = def->fss[i]; > + > + if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) { > + if (qemuVirtioFSStart(driver, vm, fs) < 0) > + return -1; > + } > + } > + > + > return ret; > } > > @@ -215,6 +227,13 @@ qemuExtDevicesStop(virQEMUDriverPtr driver, > if (slirp) > qemuSlirpStop(slirp, vm, driver, net, false); > } > + > + for (i = 0; i < def->nfss; i++) { > + virDomainFSDefPtr fs = def->fss[i]; > + > + if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) > + qemuVirtioFSStop(driver, vm, fs); Okay, hot(un)plug of VIR_DOMAIN_DEVICE_FS devices is not supported. > + } > } > > > diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c > new file mode 100644 > index 0000000000..1a3bfd53f5 > --- /dev/null > +++ b/src/qemu/qemu_virtiofs.c > @@ -0,0 +1,241 @@ [...] > +static virCommandPtr > +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; > + > + virCommandAddArg(cmd, "--syslog"); I'm not entirely sure how the virtiofsd logs, but logging into the syslog seems to be suboptimal. I think we should log into a fd/file same way as qemu does. Specifically I'm worried about the possibility to identify/match the daemon to the corresponding VM. The binary itself does not have any identifying data of the VM name and such so it might be actually hard to match up which daemon caused which log entry. Specifically since pid numbers are considered internal implementation in libvirt. > + 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); > + if (fs->cache) > + virBufferAsprintf(&opts, "cache=%s,", virDomainFSCacheModeTypeToString(fs->cache)); > + if (fs->xattr) > + virBufferAsprintf(&opts, "%sxattr,", fs->xattr == VIR_TRISTATE_SWITCH_OFF ? "no_" : ""); > + if (fs->flock) > + virBufferAsprintf(&opts, "%sflock,", fs->flock == VIR_TRISTATE_SWITCH_OFF ? "no_" : ""); > + if (fs->posix_lock) > + virBufferAsprintf(&opts, "%sposix_lock,", fs->posix_lock == VIR_TRISTATE_SWITCH_OFF ? "no_" : ""); No ternaries please. Also properly expand the conditions as I've commented in the XML patch. > + virBufferTrim(&opts, ",", -1); > + > + virCommandAddArgBuffer(cmd, &opts); > + if (cfg->virtiofsDebug) > + virCommandAddArg(cmd, "-d"); > + > + return g_steal_pointer(&cmd); > +} > + > +int > +qemuVirtioFSStart(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; > + char errbuf[1024] = { 0 }; > + 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; > + > + 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; > + > + if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd))) > + goto cleanup; > + > + virCommandSetPidFile(cmd, pidfile); > + virCommandSetErrorFD(cmd, &errfd); Is it possible to use virCommandSetErrorBuffer instead? That would solve all of the reading and pipes. > + virCommandDaemonize(cmd); > + > + 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); nit: exitstatus might not been updated thus reporting 0 might be confusing. > + 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) { Please heap-allocate 'errbuf' here. > + 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; > + } > + > + QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = g_steal_pointer(&socket_path); > + ret = 0; > + > + cleanup: > + if (socket_path) > + unlink(socket_path); > + return ret; > + > + error: > + if (pid != -1) > + virProcessKillPainfully(pid, true); > + if (pidfile) > + unlink(pidfile); > + goto cleanup; > +} > + > + > +void > +qemuVirtioFSStop(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainFSDefPtr fs) > +{ > + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); > + g_autofree char *pidfile = NULL; > + virErrorPtr orig_err; > + pid_t pid = -1; > + int rc; > + > + virErrorPreserveLast(&orig_err); > + > + if (!(pidfile = qemuVirtioFSCreatePidFilename(cfg, vm->def, fs->info.alias))) > + goto cleanup; > + > + rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL); > + if (rc >= 0 && pid != (pid_t) -1) > + virProcessKillPainfully(pid, true); Is there any caching on the daemon side which could be thrown away on an unclean shutdown? > + > + if (unlink(pidfile) < 0 && > + errno != ENOENT) { > + virReportSystemError(errno, > + _("Unable to remove stale pidfile %s"), > + pidfile); > + } > + > + if (QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock) > + unlink(QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock); > + > + cleanup: > + virErrorRestore(&orig_err); > +} [...] > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index b923590930..5a517a4982 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -496,6 +496,12 @@ testCompareXMLToArgv(const void *data) > } > } > > + for (i = 0; i < vm->def->nfss; i++) { > + virDomainFSDefPtr fs = vm->def->fss[i]; I don't think you should do this for non-virtiofs cases. > + char *s = g_strdup_printf("/tmp/lib/domain--1-guest/fs%zu.vhost-fs.sock", i); > + QEMU_DOMAIN_FS_PRIVATE(fs)->vhostuser_fs_sock = s; > + } > + > if (vm->def->vsock) { > virDomainVsockDefPtr vsock = vm->def->vsock; > qemuDomainVsockPrivatePtr vsockPriv = > -- > 2.21.0 >
