On Wed, Aug 31, 2022 at 13:40:57 -0500, Jonathon Jongsma wrote:
> For virStorageSource objects that contain an nbdkitProcess, start that
> nbdkit process to serve that network drive and then pass the nbdkit
> socket to qemu rather than sending the network url to qemu directly.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> src/qemu/qemu_block.c | 168 +++++++++++-------
> src/qemu/qemu_command.c | 4 +-
> src/qemu/qemu_domain.c | 21 ++-
> src/qemu/qemu_extdevice.c | 84 +++++++++
> src/qemu/qemu_nbdkit.c | 9 +
> src/qemu/qemu_nbdkit.h | 2 +
> ...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 +
> ...rce-curl-nbdkit-backing.x86_64-latest.args | 38 ++++
> ...isk-network-source-curl-nbdkit-backing.xml | 45 +++++
> ...work-source-curl-nbdkit.x86_64-latest.args | 50 ++++++
> .../disk-network-source-curl-nbdkit.xml | 1 +
> ...isk-network-source-curl.x86_64-latest.args | 53 ++++++
> .../disk-network-source-curl.xml | 71 ++++++++
> ...disk-network-ssh-nbdkit.x86_64-latest.args | 36 ++++
> .../disk-network-ssh-nbdkit.xml | 1 +
> tests/qemuxml2argvtest.c | 6 +
> 19 files changed, 605 insertions(+), 73 deletions(-)
> 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-backing.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/disk-network-source-curl-nbdkit-backing.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
> create mode 100644 tests/qemuxml2argvdata/disk-network-ssh-nbdkit.x86_64-latest.args
> create mode 120000 tests/qemuxml2argvdata/disk-network-ssh-nbdkit.xml
>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index b82e3311e1..4849d68039 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -439,6 +439,33 @@ qemuBlockStorageSourceGetCURLProps(virStorageSource *src,
> }
>
>
> +static virJSONValue *
> +qemuBlockStorageSourceGetNbdkitProps(virStorageSource *src)
> +{
> + qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> + virJSONValue *ret = NULL;
> + g_autoptr(virJSONValue) serverprops = NULL;
> + virStorageNetHostDef host = { .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX };
[1]
> +
> + /* srcPriv->nbdkitProcess will already be initialized if we can use nbdkit
> + * to proxy this storage source */
> + if (!(srcPriv && srcPriv->nbdkitProcess))
> + return NULL;
Here you don't return an error [2]
> +
> + host.transport = VIR_STORAGE_NET_HOST_TRANS_UNIX;
The 'transport' field is already set [1]
> + host.socket = srcPriv->nbdkitProcess->socketfile;
> + serverprops = qemuBlockStorageSourceBuildJSONSocketAddress(&host);
> +
> + if (!serverprops)
> + return NULL;
> +
> + if (virJSONValueObjectAdd(&ret, "a:server", &serverprops, NULL) < 0)
> + return NULL;
... [2] but both these cases do report an error, so you'll have hard
time actually reporting error here.
> +
> + return ret;
> +}
> +
> +
> static virJSONValue *
> qemuBlockStorageSourceGetISCSIProps(virStorageSource *src,
> bool onlytarget)
> @@ -851,69 +878,75 @@ qemuBlockStorageSourceGetBackendProps(virStorageSource *src,
> return NULL;
>
> case VIR_STORAGE_TYPE_NETWORK:
> - switch ((virStorageNetProtocol) src->protocol) {
> - case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
I'll post a patch that factores out the formatting of props for network
storage so that this patch doesn't have to change it.
> - driver = "gluster";
> - if (!(fileprops = qemuBlockStorageSourceGetGlusterProps(src, onlytarget)))
> - return NULL;
[...]
> @@ -1799,6 +1832,8 @@ qemuBlockGetBackingStoreString(virStorageSource *src,
> virJSONValue *props = NULL;
> g_autoptr(virURI) uri = NULL;
> g_autofree char *backingJSON = NULL;
> + qemuDomainStorageSourcePrivate *srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> + bool useNbdkit = srcPriv && srcPriv->nbdkitProcess;
>
> if (!src->sliceStorage) {
> if (virStorageSourceIsLocalStorage(src)) {
> @@ -1817,7 +1852,8 @@ qemuBlockGetBackingStoreString(virStorageSource *src,
> src->ncookies == 0 &&
> src->sslverify == VIR_TRISTATE_BOOL_ABSENT &&
> src->timeout == 0 &&
> - src->readahead == 0) {
> + src->readahead == 0 &&
> + !useNbdkit) {
This doesn't make sense. Regardless of how the disk is accessed (via
nbdkit or not) the qemuBlockGetBackingStoreString must return the same
string.
The returned string is used to record into the qcow2 headers when doing
snapshots and other operations so any transient method to access them
will not be there once the file is inspected again.
Thus even when the JSON props are returned naturally it must not contain
any trace of nbdkit when called via this entry point.
> @@ -2224,6 +2261,13 @@ qemuBlockStorageSourceCreateGetStorageProps(virStorageSource *src,
> break;
>
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + if (srcPriv->nbdkitProcess) {
> + /* disk creation not yet supported with nbdkit, and even if it
> + * was supported, it would not be done with blockdev-create
> + * props */
> + return 0;
> + }
> +
> driver = "ssh";
> if (!(location = qemuBlockStorageSourceGetSshProps(src)))
> return -1;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a31b8ee438..b941f0b3ac 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10372,7 +10372,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src,
> return -1;
>
> if (srcpriv) {
> - if (srcpriv->secinfo &&
> + if (!srcpriv->nbdkitProcess && srcpriv->secinfo &&
The code should avoid setting 'secinfo' if nbdkit is used rather than
patching it here.
> qemuBuildSecretInfoProps(srcpriv->secinfo, &data->authsecretProps) < 0)
> return -1;
>
> @@ -10380,7 +10380,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSource *src,
> qemuBuildSecretInfoProps(srcpriv->encinfo, &data->encryptsecretProps) < 0)
> return -1;
>
> - if (srcpriv->httpcookie &&
> + if (!srcpriv->nbdkitProcess && srcpriv->httpcookie &&
> qemuBuildSecretInfoProps(srcpriv->httpcookie, &data->httpcookiesecretProps) < 0)
Same here.
> return -1;
>
[...]
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index b8e3c1000a..ffab7d048b 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -164,6 +164,25 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver,
> }
>
>
> +/* recursively start nbdkit for backing chain of src */
> +static int qemuExtDevicesStartNbdkit(virQEMUDriver *driver,
> + virDomainObj *vm,
> + virStorageSource *src)
Wrong and inconsistent header style.
> +{
> + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> + if (src->backingStore)
> + if (qemuExtDevicesStartNbdkit(driver, vm, src->backingStore) < 0)
> + return -1;
Preferrably use a loop instead of recursion.
> +
> + if (priv && priv->nbdkitProcess &&
> + qemuNbdkitProcessStart(priv->nbdkitProcess, vm, driver) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> int
> qemuExtDevicesStart(virQEMUDriver *driver,
> virDomainObj *vm,
> @@ -218,6 +237,34 @@ qemuExtDevicesStart(virQEMUDriver *driver,
> return -1;
> }
>
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDef *disk = def->disks[i];
> + if (qemuExtDevicesStartNbdkit(driver, vm, disk->src) < 0)
> + return -1;
> + }
> +
> + if (def->os.loader && def->os.loader->nvram) {
> + if (qemuExtDevicesStartNbdkit(driver, vm, def->os.loader->nvram) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> +/* recursively stop nbdkit processes for backing chain of src */
> +static int qemuExtDevicesStopNbdkit(virStorageSource *src)
Wrong and inconsistent header style.
> +{
> + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> +
> + if (src->backingStore)
> + if (qemuExtDevicesStopNbdkit(src->backingStore) < 0)
> + return -1;
Preferrably use a loop instead of recursion.
> +
> + if (priv && priv->nbdkitProcess &&
> + qemuNbdkitProcessStop(priv->nbdkitProcess) < 0)
> + return -1;
> +
> return 0;
> }
>
> @@ -291,6 +346,24 @@ qemuExtDevicesHasDevice(virDomainDef *def)
> }
>
>
> +/* recursively setup nbdkit cgroups for backing chain of src */
> +static int qemuExtDevicesSetupCgroupNbdkit(virStorageSource *src,
> + virCgroup *cgroup)
Wrong style of header alignment..
> +{
> + qemuDomainStorageSourcePrivate *priv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
... too much indent.
> +
> + if (src->backingStore)
> + if (qemuExtDevicesSetupCgroupNbdkit(src->backingStore, cgroup) < 0)
> + return -1;
Preferrably use a loop instead of recursion.
> +
> + if (priv && priv->nbdkitProcess &&
> + qemuNbdkitProcessSetupCgroup(priv->nbdkitProcess, cgroup) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> int
> qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
> virDomainObj *vm,
