8:42 a.m.
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