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

[Ján Tomko]

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.

>
>> +            goto cleanup;
>> +    } else {
>> +        if ((logfd = open(logpath, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR)) < 0) {
>> +            virReportSystemError(errno, _("failed to create logfile %s"),
>> +                                 logpath);
>> +            goto cleanup;
>> +        }
>> +        if (virSetCloseExec(logfd) < 0) {
>> +            virReportSystemError(errno, _("failed to set close-on-exec flag on %s"),
>> +                                 logpath);
>> +            goto error;
>> +        }
>> +    }
>> +
>> +    if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd)))
>> +        goto cleanup;
>> +
>> +    /* so far only running as root is supported */
>> +    virCommandSetUID(cmd, 0);
>> +    virCommandSetGID(cmd, 0);
>
>Is it necessary to special-case this for cases when we run the session
>connection? Or is it necessary to forbid usage of virtiofs?
>

Right, session users deserve a nicer error message until virtiofsd
learns to run as non-root.

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200226/4ad4569e/attachment-0001.sig>