[libvirt PATCH 3/3] WIP: use nbdkit for remote disk sources

[Michal Prívozník]

On 6/22/22 23:26, Jonathon Jongsma wrote:
> ---

When you send this for review, please split it into more patches.
There's too much going on in this single patch.

>  include/libvirt/virterror.h                   |   1 +
>  po/POTFILES                                   |   1 +
>  src/qemu/meson.build                          |   1 +
>  src/qemu/qemu_block.c                         |  64 +-
>  src/qemu/qemu_block.h                         |   1 +
>  src/qemu/qemu_command.c                       |  26 +-
>  src/qemu/qemu_conf.c                          |  19 +
>  src/qemu/qemu_conf.h                          |   5 +
>  src/qemu/qemu_domain.c                        | 110 ++-
>  src/qemu/qemu_domain.h                        |   5 +
>  src/qemu/qemu_driver.c                        |   4 +-
>  src/qemu/qemu_extdevice.c                     |  25 +
>  src/qemu/qemu_nbdkit.c                        | 629 ++++++++++++++++++
>  src/qemu/qemu_nbdkit.h                        |  89 +++
>  src/qemu/qemu_validate.c                      |  22 +-
>  src/qemu/qemu_validate.h                      |   4 +-
>  src/util/virerror.c                           |   1 +
>  tests/qemublocktest.c                         |   8 +-
>  tests/qemustatusxml2xmldata/modern-in.xml     |   1 -
>  ...sk-cdrom-network-nbdkit.x86_64-latest.args |  42 ++
>  .../disk-cdrom-network-nbdkit.xml             |   1 +
>  ...isk-network-http-nbdkit.x86_64-latest.args |  45 ++
>  .../disk-network-http-nbdkit.xml              |   1 +
>  ...work-source-curl-nbdkit.x86_64-latest.args |  49 ++
>  .../disk-network-source-curl-nbdkit.xml       |   1 +
>  ...isk-network-source-curl.x86_64-latest.args |  53 ++
>  .../disk-network-source-curl.xml              |  71 ++
>  tests/qemuxml2argvtest.c                      |  12 +
>  tests/testutilsqemu.c                         |  16 +
>  tests/testutilsqemu.h                         |   4 +
>  30 files changed, 1278 insertions(+), 33 deletions(-)
>  create mode 100644 src/qemu/qemu_nbdkit.c
>  create mode 100644 src/qemu/qemu_nbdkit.h
>  create mode 100644 tests/qemuxml2argvdata/disk-cdrom-network-nbdkit.x86_64-latest.args
>  create mode 120000 tests/qemuxml2argvdata/disk-cdrom-network-nbdkit.xml
>  create mode 100644 tests/qemuxml2argvdata/disk-network-http-nbdkit.x86_64-latest.args
>  create mode 120000 tests/qemuxml2argvdata/disk-network-http-nbdkit.xml
>  create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit.x86_64-latest.args
>  create mode 120000 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit.xml
>  create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl.xml
> 


> +
> +
> +int
> +qemuNbdkitProcessSetupCgroup(qemuNbdkitProcess *proc,
> +                             virCgroup *cgroup)
> +{
> +    return virCgroupAddProcess(cgroup, proc->pid);
> +}
> +
> +
> +/* FIXME: selinux permissions errors */
> +int
> +qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
> +                       virDomainObj *vm,
> +                       virQEMUDriver *driver)
> +{
> +    g_autoptr(virCommand) cmd = NULL;
> +    int rc;
> +    int exitstatus = 0;
> +    int cmdret = 0;
> +    unsigned int loops = 0;
> +    int errfd = -1;
> +
> +    if (qemuNbdkitProcessForkCookieHandler(proc) < 0)
> +        return -1;
> +

Creating an extra process so that a buffer can be written into an FD
looks like an overkill.

> +    if (qemuNbdkitProcessForkPasswordHandler(proc) < 0)
> +        return -1;

Same here. What happens if you'd just write into these sockets (btw. can
it be just a pipe?) after virCommandRun()?

> +
> +    if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
> +        return -1;
> +> +    virCommandSetErrorFD(cmd, &errfd);
> +
> +    if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
> +        goto error;
> +
> +    if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, &exitstatus, &cmdret) < 0)
> +        goto error;
> +
> +    if (cmdret < 0 || exitstatus != 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not start 'nbdkit'. exitstatus: %d"), exitstatus);
> +        goto error;
> +    }
> +

This or .. [1]

> +    /* Wait for the pid file to appear. The file doesn't appear until nbdkit is
> +     * ready to accept connections to the socket */
> +    while (!virFileExists(proc->pidfile)) {

This relies on executed binary to create pidfile. I find it more robust
when the pidfile is created by virCommand*() - also because it's going
to be locked and we can detect later on whether it's stale or not. But
more importantly - the pidfile is written before exec(). You can take
inspiration from qemuDBusStart().

> +        VIR_DEBUG("waiting for nbdkit pidfile %s: %u", proc->pidfile, loops);
> +        /* wait for 100ms before checking again, but don't do it for ever */
> +        if (errno == ENOENT && loops < 10) {
> +            g_usleep(100 * 1000);
> +            loops++;
> +        } else {
> +            char errbuf[1024] = { 0 };
> +            if (errfd && saferead(errfd, errbuf, sizeof(errbuf) - 1) > 0) {
> +                    virReportError(VIR_ERR_OPERATION_FAILED,
> +                                   _("nbdkit failed to start: %s"), errbuf);
> +            } else {
> +                virReportSystemError(errno, "%s",
> +                                     _("Gave up waiting for nbdkit to start"));
> +            }
> +            goto error;
> +        }
> +    }
> +
> +    if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
> +        virReportSystemError(-rc,
> +                             _("Failed to read pidfile %s"),
> +                             proc->pidfile);
> +        goto error;
> +    }
> +

1: ... this looks like a good place to write those secrets. Moreover, if
we'd get error writing into the pipe we know the process died.

I don't feel confident reviewing the storage part of the feature, sorry.

Michal