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

Daniel P. Berrangé berrange at redhat.com
Wed Feb 26 10:41:11 UTC 2020


On Wed, Feb 26, 2020 at 11:34:06AM +0100, Ján Tomko wrote:
> 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 at redhat.com>
> > > ---
> > 
> > [...]
> > 
> > > 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.

Essentially the UUID/name are just a way to associate the log file(s) with
what "thing" is owning them.

We already have virtlogd handling multiple logs from the same QEMU - the
main QEMU stderr, and the file base logs for character devices.

Having another log file open for virtiofs, slirp, swtpm, etc, all with
the same UUID+name makes total sense.


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 :|




More information about the libvir-list mailing list