Re: [libvirt] [PATCH 8/9] wip: start virtiofsd
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 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
Re: [libvirt] [PATCH 8/9] wip: start virtiofsd
On Fri, Nov 01, 2019 at 01:16:06PM +0100, Ján Tomko wrote:
> Start virtiofsd for each 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
>
> +#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(, "%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(, "%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, ) < 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, );
> +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
Re: [libvirt] [PATCH 8/9] wip: start virtiofsd
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 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
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] wip: start virtiofsd
On Fri, Nov 1, 2019 at 1:18 PM Ján Tomko 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
[libvirt] [PATCH 8/9] wip: start virtiofsd
Start virtiofsd for each 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
+#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(, "%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(, "%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, ) < 0)
+goto cleanup;
+
+/* TODO: configure path */
+if (!(cmd = virCommandNew("/usr/libexec/virtiofsd")))
+goto cleanup;
+
+virCommandSetPidFile(cmd, pidfile);
+virCommandSetErrorFD(cmd, );
+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, );
+
+if (cmdret < 0 || exitstatus != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not start 'virtiofsd'. exitstatus: %d"),
exitstatus);
+goto error;
+}
+
+rc = virPidFileReadPath(pidfile, );
+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:
