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 */
};
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;
+
+ return virPidFileBuildPath(cfg->stateDir, name);
+}
+
+
+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");
+}
+
+
+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;
+
+ 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;
+
+ /* TODO: configure path */
+ if (!(cmd = virCommandNew("/usr/libexec/virtiofsd")))
+ goto cleanup;
+
+ virCommandSetPidFile(cmd, pidfile);
+ virCommandSetErrorFD(cmd, &errfd);
+ virCommandDaemonize(cmd);
+
+ virCommandAddArg(cmd, "--syslog");
+ 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");
+
+ 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;
+ }
+
+ 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;
+}
+
+
+static void
+qemuExtVirtioFSdStop(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 = qemuExtVirtioFSCreatePidFilename(cfg, vm->def, fs->info.alias)))
+ goto cleanup;
+
+ rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
+ if (rc >= 0 && pid != (pid_t) -1)
+ virProcessKillPainfully(pid, true);
+
+ if (unlink(pidfile) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno,
+ _("Unable to remove stale pidfile %s"),
+ pidfile);
+ }
+
+ if (fs->vhost_user_fs_path)
+ unlink(fs->vhost_user_fs_path);
+
+ cleanup:
+ virErrorRestore(&orig_err);
+}
+
int
qemuExtDevicesStart(virQEMUDriverPtr driver,
@@ -184,6 +357,17 @@ qemuExtDevicesStart(virQEMUDriverPtr driver,
return -1;
}
+ for (i = 0; i < def->nfss; i++) {
+ /* FIXME: use private data */
+ virDomainFSDefPtr fs = def->fss[i];
+
+ if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIO_FS) {
+ if (qemuExtVirtioFSdStart(driver, vm, fs) < 0)
+ return -1;
+ }
+ }
+
+
return ret;
}
@@ -215,6 +399,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_VIRTIO_FS)
+ qemuExtVirtioFSdStop(driver, vm, fs);
+ }
}
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 24df0a48e7..bf3366f953 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -501,6 +501,12 @@ testCompareXMLToArgv(const void *data)
}
}
+ for (i = 0; i < vm->def->nfss; i++) {
+ virDomainFSDefPtr fs = vm->def->fss[i];
+ char *s = g_strdup_printf("/tmp/lib/domain--1-guest/fs%zu.vhost-fs.sock", i);
+ fs->vhost_user_fs_path = s;
+ }
+
if (vm->def->vsock) {
virDomainVsockDefPtr vsock = vm->def->vsock;
qemuDomainVsockPrivatePtr vsockPriv =
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Nov 1, 2019 at 1:18 PM Ján Tomko <jtomko@redhat.com> wrote:
> + 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;
qemuSecurityClearSocketLabel() is not called in the
qemuOpenChrChardevUNIXSocket() error code path. Is this correct?
> +static void
> +qemuExtVirtioFSdStop(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainFSDefPtr fs)
> +{
The daemon stops automatically when the vhost-user socket is closed by
QEMU. Is it necessary to implement an explicit stop function?
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 04, 2019 at 10:06:40AM +0100, Stefan Hajnoczi wrote:
>On Fri, Nov 1, 2019 at 1:18 PM Ján Tomko <jtomko@redhat.com> wrote:
>> + 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;
>
>qemuSecurityClearSocketLabel() is not called in the
>qemuOpenChrChardevUNIXSocket() error code path. Is this correct?
>
That's an oversight, thanks for catching that.
>> +static void
>> +qemuExtVirtioFSdStop(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + virDomainFSDefPtr fs)
>> +{
>
>The daemon stops automatically when the vhost-user socket is closed by
>QEMU. Is it necessary to implement an explicit stop function?
>
I hope not, but thought it was better to have it than possibly leaving
leftover daemons.
Jano
>Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 04, 2019 at 10:06:40AM +0100, Stefan Hajnoczi wrote:
> On Fri, Nov 1, 2019 at 1:18 PM Ján Tomko <jtomko@redhat.com> wrote:
> > + 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;
>
> qemuSecurityClearSocketLabel() is not called in the
> qemuOpenChrChardevUNIXSocket() error code path. Is this correct?
>
> > +static void
> > +qemuExtVirtioFSdStop(virQEMUDriverPtr driver,
> > + virDomainObjPtr vm,
> > + virDomainFSDefPtr fs)
> > +{
>
> The daemon stops automatically when the vhost-user socket is closed by
> QEMU. Is it necessary to implement an explicit stop function?
That's good, but we've generally wanted to be explicit about cleaning
things up to cope with unexpected circumstances. In particular QEMU
can get itself stuck as a zombie if there's a dead disk, so it is
worth tearing down virtiofsd explicitly.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.