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
