[libvirt PATCHv2 08/10] qemu: add code for handling virtiofsd
Peter Krempa
pkrempa at redhat.com
Fri Jan 24 09:25:39 UTC 2020
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 at redhat.com>
> ---
> 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
>
More information about the libvir-list
mailing list