[libvirt PATCH v4 18/31] qemu: include nbdkit state in private xml

[Peter Krempa]

On Fri, Jan 20, 2023 at 16:03:12 -0600, Jonathon Jongsma wrote:
> Add xml to the private data for a disk source to represent the nbdkit
> process so that the state can be re-created if the libvirt daemon is
> restarted. Format:
> 
>    <nbdkit>
>      <pidfile>/path/to/nbdkit.pid</pidfile>
>      <socketfile>/path/to/nbdkit.socket</socketfile>
>    </nbdkit>
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  src/qemu/qemu_domain.c                    | 48 ++++++++++++++
>  src/qemu/qemu_nbdkit.c                    | 77 +++++++++++++++++++++++
>  src/qemu/qemu_nbdkit.h                    |  8 +++
>  src/qemu/qemu_process.c                   | 11 ++++
>  tests/qemustatusxml2xmldata/modern-in.xml |  4 ++
>  5 files changed, 148 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0e3eaf49f8..28d4bddf14 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1911,6 +1911,29 @@ qemuStorageSourcePrivateDataAssignSecinfo(qemuDomainSecretInfo **secinfo,
>  }
>  
>  
> +static int
> +qemuStorageSourcePrivateDataParseNbdkit(xmlNodePtr node,
> +                                        xmlXPathContextPtr ctxt,
> +                                        virStorageSource *src)
> +{
> +    g_autofree char *pidfile = NULL;
> +    g_autofree char *socketfile = NULL;
> +    VIR_XPATH_NODE_AUTORESTORE(ctxt);
> +
> +    ctxt->node = node;
> +
> +    if (!(pidfile = virXPathString("string(./pidfile)", ctxt)))
> +        return -1;

The XPath apis are a bit misleading/broken. For the basic case, when you
have a correct XPath, but the element is missing they *don't* actually
report an error. They report errors only form programming errors.

Thus you must add error reports here.

> +
> +    if (!(socketfile = virXPathString("string(./socketfile)", ctxt)))
> +        return -1;
> +
> +    qemuNbdkitReconnectStorageSource(src, pidfile, socketfile);
> +
> +    return 0;
> +}

[...]

> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 00ca945904..2b26e9bc08 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -627,6 +627,83 @@ qemuNbdkitProcessNew(virStorageSource *source,

[...]

> +static int
> +qemuNbdkitStorageSourceManageProcessOne(virStorageSource *source)
> +{
> +    qemuDomainStorageSourcePrivate *srcpriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(source);
> +    qemuNbdkitProcess *proc;
> +
> +    if (!srcpriv)
> +        return 0;
> +
> +    proc = srcpriv->nbdkitProcess;
> +
> +    if (proc) {
> +        if (proc->pid <= 0) {
> +            if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0)
> +                return -1;
> +        }
> +
> +        if (virProcessKill(proc->pid, 0) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("nbdkit process %i is not alive"), proc->pid);
> +            return -1;

In later patches you add code for monitoring the nbdkit process which is
actually mean to restart it, so we should not kill the VM if 'nbdkit'
died while libvirt was not looking.

This is not fixed even in 24/31 which adds the monitoring bit and thus
could restart nbdkit.

> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/**
> + * qemuNbdkitStorageSourceManageProcess:
> + * @source: a storage source
> + * @vm: the vm that owns this storage source
> + *
> + * This function re-enables monitoring of any nbdkit processes associated with the backing chain of
> + * @source. It is intended to be called after libvirt restarts and has loaded its current state from
> + * disk and is attempting to re-connect to active domains.
> + */
> +int
> +qemuNbdkitStorageSourceManageProcess(virStorageSource *source)
> +{
> +    virStorageSource *backing;
> +    for (backing = source->backingStore; backing != NULL; backing = backing->backingStore) {

if you initialize 'backing' to 'source' instead ...

> +        if (qemuNbdkitStorageSourceManageProcessOne(backing) < 0)
> +            return -1;
> +    }
> +
> +    return qemuNbdkitStorageSourceManageProcessOne(source);

You don't have to have this extra invocation, which also makes the order
the processes are checked weird ... eg. backing of the top node only is
processed before parent, but backing of a child note is processed
after the parent

> +}

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6d1751b5d7..7ec31ef6ac 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -9047,6 +9047,17 @@ qemuProcessReconnect(void *opaque)
>          }
>      }
>  
> +    for (i = 0; i < obj->def->ndisks; i++) {
> +        virDomainDiskDef *disk = obj->def->disks[i];
> +        if (qemuNbdkitStorageSourceManageProcess(disk->src) < 0)
> +            goto error;

This kills the VM if nbdkit is not running.

> +    }
> +
> +    if (obj->def->os.loader && obj->def->os.loader->nvram) {
> +        if (qemuNbdkitStorageSourceManageProcess(obj->def->os.loader->nvram) < 0)
> +            goto error;
> +    }
> +