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