Re: [libvirt PATCHv4 11/15] qemu: add code for handling virtiofsd

On Wed, Feb 26, 2020 at 09:42:04AM +0100, Peter Krempa wrote: > Explicitly CCing danpb to clarify usage of the logging daemon. > > On Thu, Feb 20, 2020 at 15:32:48 +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 a43efa34c7d7b628cbf1ec0fe60043e5c91043ea > > > > Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; > > --- > > [...] > > > diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c > > new file mode 100644 > > index 0000000000..600a3644bd > > --- /dev/null > > +++ b/src/qemu/qemu_virtiofs.c > > @@ -0,0 +1,300 @@ > > [...] > > > +char * > > +qemuVirtioFSCreatePidFilename(virQEMUDriverConfigPtr cfg, > > + const virDomainDef *def, > > + const char *alias) > > +{ > > + g_autofree char *shortName = NULL; > > + g_autofree char *name = NULL; > > + > > + if (!(shortName = virDomainDefGetShortName(def))) > > + return NULL; > > + > > + name = g_strdup_printf("%s-%s-virtiofsd", shortName, alias); > > + > > + return virPidFileBuildPath(cfg->stateDir, name); > > +} > > Why do we want to store the pid file here? > > > + > > + > > +char * > > +qemuVirtioFSCreateSocketFilename(virDomainObjPtr vm, > > + const char *alias) > > +{ > > + qemuDomainObjPrivatePtr priv = vm->privateData; > > + > > + return virFileBuildPath(priv->libDir, alias, "-virtiofsd.sock"); > > Shouldn't we put it here as well? It's a sub-process of the domain > itself and doesn't make much sense to treat it as the qemu pid file. > Right, libDir sounds like a better location. [...] > > + > > + logpath = qemuVirtioFSCreateLogFilename(cfg, vm->def, fs->info.alias); > > + > > + if (cfg->stdioLogD) { > > + if ((logfd = virLogManagerDomainOpenLogFile(logManager, > > + "qemu", > > + vm->def->uuid, > > + vm->def->name, > > + logpath, > > + 0, > > + NULL, NULL)) < 0) > > Hmm, I'm not sure now what the intented usage of uuid/domname was and > there's no docs. > > Daniel, please review this. > Grepping for uuid in src/logging, this seems to be only used when saving and loading virtlockd's state. But virtiofsd's lifecycle is tied to the domain, so I don't think it should get its own uuid.