[PATCH 00/17] qemu: Prepare block device setup for removing the 'raw' driver ('raw' driver removal part 3)
This series prepares the setup of the block device backend for removal of the raw driver from the block graph, and actually removes it from the migration NBD connection. Unfortunately for normal usage it's not yet possible as qemu then records the protocol driver name in the backing file format field which would break with older versions of libvirt (see my other series). Peter Krempa (17): qemu: block: Introduce qemuBlockStorageSourceGetSliceNodename qemu: block: Use qemuBlockStorageSourceNeedsStorageSliceLayer only for setup qemu: block: Introduce helper for deciding when a 'format' layer is needed qemuBlockStorageSourceGetBlockdevStorageSliceProps: Allow turning the slice layer into effective blockdev layer qemuBlockStorageSourceAttachPrepareBlockdev: Prepare for optionally missing format layer qemuBlockStorageSourceDetachPrepare: Prepare for possibly missing 'format' layer qemuDomainPrepareStorageSourceBlockdevNodename: Restructure code to allow missing 'format' layer qemuBlockStorageSourceGetEffectiveNodename: Prepare for missing 'format' driver qemu: block: Extract logic from qemuBlockReopenReadWrite/ReadOnly qemu: block: Absorb logic from qemuBlockReopenFormat to qemuBlockReopenAccess qemu: monitor: Sanitize arguments of qemuMonitorBlockdevReopen testQemuMonitorJSONBlockdevReopen: Don't use qemuBlockReopenFormatMon qemu: block: Absorb qemuBlockReopenFormatMon into qemuBlockReopenAccess qemuBlockReopenAccess: prepare for removal of 'raw' format layer qemuMigrationSrcNBDCopyCancel: Use qemuBlockStorageSourceAttachRollback to detach migration NBD blockdevs qemu: block: Remove unused qemuBlockStorageSourceDetachOneBlockdev qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource: Don't setup 'raw' layer for migration NBD connection src/qemu/qemu_block.c | 257 ++++++++++++++++++----------------- src/qemu/qemu_block.h | 16 +-- src/qemu/qemu_domain.c | 17 ++- src/qemu/qemu_migration.c | 19 ++- src/qemu/qemu_monitor.c | 7 +- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 6 +- src/qemu/qemu_monitor_json.h | 2 +- tests/qemumonitorjsontest.c | 10 +- 9 files changed, 185 insertions(+), 151 deletions(-) -- 2.42.0
The helper retrieves the nodename of the slice layer if it's present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 23 ++++++++++++++++++++--- src/qemu/qemu_block.h | 3 +++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index fab122942a..ea1af61561 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -101,6 +101,22 @@ qemuBlockStorageSourceGetEffectiveNodename(virStorageSource *src) } +/** + * qemuBlockStorageSourceGetSliceNodename: + * + * Gets the nodename corresponding to the storage slice layer. Returns NULL + * when there is no explicit storage slice layer. + */ +const char * +qemuBlockStorageSourceGetSliceNodename(virStorageSource *src) +{ + if (!src->sliceStorage) + return NULL; + + return src->sliceStorage->nodename; +} + + /** * qemuBlockStorageSourceGetEffectiveStorageNodename: * @src: virStorageSource to get the effective nodename of @@ -111,9 +127,10 @@ qemuBlockStorageSourceGetEffectiveNodename(virStorageSource *src) const char * qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src) { - if (src->sliceStorage && - src->sliceStorage->nodename) - return src->sliceStorage->nodename; + const char *slice = qemuBlockStorageSourceGetSliceNodename(src); + + if (slice) + return slice; return src->nodenamestorage; } diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 5c784a4386..85616a140d 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -40,6 +40,9 @@ qemuBlockStorageSourceGetEffectiveStorageNodename(virStorageSource *src); const char * qemuBlockStorageSourceGetStorageNodename(virStorageSource *src); +const char * +qemuBlockStorageSourceGetSliceNodename(virStorageSource *src); + const char * qemuBlockStorageSourceGetFormatNodename(virStorageSource *src); -- 2.42.0
Add a note stating that qemuBlockStorageSourceNeedsStorageSliceLayer must be used only when setting up a new blockdev, any other case when the device might been already set up must use the existance of the nodename to do so. Adjust qemuBlockStorageSourceAttachPrepareBlockdev to do so and refactor qemuBlockStorageSourceDetachPrepare to use the same logic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index ea1af61561..1a718ae82b 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1535,11 +1535,9 @@ qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src, data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src); - if (qemuBlockStorageSourceNeedsStorageSliceLayer(src)) { + if ((data->storageSliceNodeName = qemuBlockStorageSourceGetSliceNodename(src))) { if (!(data->storageSliceProps = qemuBlockStorageSourceGetBlockdevStorageSliceProps(src))) return NULL; - - data->storageSliceNodeName = src->sliceStorage->nodename; } return g_steal_pointer(&data); @@ -1756,13 +1754,8 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src) data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); data->storageAttached = true; - /* 'raw' format doesn't need the extra 'raw' layer when slicing, thus - * the nodename is NULL */ - if (src->sliceStorage && - src->sliceStorage->nodename) { - data->storageSliceNodeName = src->sliceStorage->nodename; + if ((data->storageSliceNodeName = qemuBlockStorageSourceGetSliceNodename(src))) data->storageSliceAttached = true; - } if (src->pr && !virStoragePRDefIsManaged(src->pr)) @@ -3264,6 +3257,12 @@ qemuBlockReopenReadOnly(virDomainObj *vm, * * Returns true if @src requires an extra 'raw' layer for handling of the storage * slice. + * + * Important: This helper must be used only for decisions when setting up a + * '-blockdev' backend in which case the storage slice layer node name will be + * populated. + * Any cases when the backend can be already in use must decide based on the + * existence of the storage slice layer nodename. */ bool qemuBlockStorageSourceNeedsStorageSliceLayer(const virStorageSource *src) -- 2.42.0
On a Friday in 2023, Peter Krempa wrote:
Add a note stating that qemuBlockStorageSourceNeedsStorageSliceLayer must be used only when setting up a new blockdev, any other case when the device might been already set up must use the existance of the
existence
nodename to do so.
Adjust qemuBlockStorageSourceAttachPrepareBlockdev to do so and refactor qemuBlockStorageSourceDetachPrepare to use the same logic.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
Jano
The 'format' layer is not required in certain cases. As the logic for this will be a bit more involved create a helper function to do the decision. For now we'll keep to always format the 'format' -blockdev layer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 19 +++++++++++++++++++ src/qemu/qemu_block.h | 3 +++ 2 files changed, 22 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 1a718ae82b..123e764e63 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3281,6 +3281,25 @@ qemuBlockStorageSourceNeedsStorageSliceLayer(const virStorageSource *src) } +/** + * qemuBlockStorageSourceNeedsFormatLayer: + * @src: storage source + * + * Returns true if configuration of @src requires a 'format' layer -blockdev. + * + * Important: This helper must be used only for decisions when setting up a + * '-blockdev' backend in which case the format layer node name will be populated. + * Any cases when the backend can be already in use must decide based on the + * existence of the format layer nodename. + */ +bool +qemuBlockStorageSourceNeedsFormatLayer(const virStorageSource *src G_GNUC_UNUSED) +{ + /* Currently we always create a 'format' layer */ + return true; +} + + /** * qemuBlockStorageSourceGetCookieString: * @src: storage source diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 85616a140d..dcd8a6ed6c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -280,6 +280,9 @@ qemuBlockReopenReadOnly(virDomainObj *vm, bool qemuBlockStorageSourceNeedsStorageSliceLayer(const virStorageSource *src); +bool +qemuBlockStorageSourceNeedsFormatLayer(const virStorageSource *src); + char * qemuBlockStorageSourceGetCookieString(virStorageSource *src); -- 2.42.0
Allow using the slice layer as effective layer once we stop formatting the unnecessary 'raw' driver. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 123e764e63..f7e912ece0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1448,8 +1448,18 @@ qemuBlockStorageSourceGetFormatProps(virStorageSource *src, } +/** + * qemuBlockStorageSourceGetBlockdevStorageSliceProps: + * @src: storage source object + * @effective: Whether this blockdev will be the 'effective' layer of @src + * + * Formats the JSON object representing -blockdev configuration required to + * configure a 'slice' of @src. If @effective is true, the slice layer is the + * topmost/effective blockdev layer of @src. + */ static virJSONValue * -qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src) +qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src, + bool effective) { g_autoptr(virJSONValue) props = NULL; @@ -1464,7 +1474,7 @@ qemuBlockStorageSourceGetBlockdevStorageSliceProps(virStorageSource *src) if (qemuBlockStorageSourceAddBlockdevCommonProps(&props, src, src->sliceStorage->nodename, - false) < 0) + effective) < 0) return NULL; return g_steal_pointer(&props); @@ -1536,7 +1546,7 @@ qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src, data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src); if ((data->storageSliceNodeName = qemuBlockStorageSourceGetSliceNodename(src))) { - if (!(data->storageSliceProps = qemuBlockStorageSourceGetBlockdevStorageSliceProps(src))) + if (!(data->storageSliceProps = qemuBlockStorageSourceGetBlockdevStorageSliceProps(src, false))) return NULL; } -- 2.42.0
Restructure the code logic so that the function is prepared for the possibility that the 'format' blockdev layer may be missing if not needed. To achieve this we need to introduce logic that selects which node (format/slice/storage) becomes the effective node and thus formats the correct set of arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index f7e912ece0..749cd9fdac 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1533,23 +1533,35 @@ qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSource *src, virStorageSource *backingStore) { g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; + bool effective = true; unsigned int backendpropsflags = 0; data = g_new0(qemuBlockStorageSourceAttachData, 1); - if (!(data->formatProps = qemuBlockStorageSourceGetFormatProps(src, backingStore)) || - !(data->storageProps = qemuBlockStorageSourceGetBackendProps(src, - backendpropsflags))) - return NULL; + if (qemuBlockStorageSourceGetFormatNodename(src)) { + if (!(data->formatProps = qemuBlockStorageSourceGetFormatProps(src, backingStore))) + return NULL; - data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); - data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src); + data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src); + + effective = false; + } if ((data->storageSliceNodeName = qemuBlockStorageSourceGetSliceNodename(src))) { - if (!(data->storageSliceProps = qemuBlockStorageSourceGetBlockdevStorageSliceProps(src, false))) + if (!(data->storageSliceProps = qemuBlockStorageSourceGetBlockdevStorageSliceProps(src, effective))) return NULL; + + effective = false; } + if (effective) + backendpropsflags = QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE; + + if (!(data->storageProps = qemuBlockStorageSourceGetBackendProps(src, backendpropsflags))) + return NULL; + + data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); + return g_steal_pointer(&data); } -- 2.42.0
Setup the data for detaching of the 'format' layer only when it's present. Restructure the logic to follow the same order as qemuBlockStorageSourceAttachPrepareBlockdev in terms of format/slice/storage -blockdev objects, and drop the now-misleading comment for 'slice' of raw disks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 749cd9fdac..7137604e36 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1771,14 +1771,15 @@ qemuBlockStorageSourceDetachPrepare(virStorageSource *src) data = g_new0(qemuBlockStorageSourceAttachData, 1); - data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src); - data->formatAttached = true; - data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); - data->storageAttached = true; + if ((data->formatNodeName = qemuBlockStorageSourceGetFormatNodename(src))) + data->formatAttached = true; if ((data->storageSliceNodeName = qemuBlockStorageSourceGetSliceNodename(src))) data->storageSliceAttached = true; + data->storageNodeName = qemuBlockStorageSourceGetStorageNodename(src); + data->storageAttached = true; + if (src->pr && !virStoragePRDefIsManaged(src->pr)) data->prmgrAlias = g_strdup(src->pr->mgralias); -- 2.42.0
Similarly to other bits of code, we don't need to setup the format layer if it will not be formatted. Add logic which uses qemuBlockStorageSourceNeedsFormatLayer to see whether the setup of the format node is needed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ae19ce884b..6d3161c5d7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11145,11 +11145,21 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, virQEMUDriverConfig *cfg) { char *nodestorage = g_strdup_printf("%s-storage", nodenameprefix); - char *nodeformat = g_strdup_printf("%s-format", nodenameprefix); + const char *encryptionAlias = nodestorage; /* qemuBlockStorageSourceSetStorageNodename steals 'nodestorage' */ qemuBlockStorageSourceSetStorageNodename(src, nodestorage); - qemuBlockStorageSourceSetFormatNodename(src, nodeformat); + + if (qemuBlockStorageSourceNeedsFormatLayer(src)) { + char *nodeformat = g_strdup_printf("%s-format", nodenameprefix); + + qemuBlockStorageSourceSetFormatNodename(src, nodeformat); + + encryptionAlias = nodeformat; + } + + if (qemuDomainSecretStorageSourcePrepareEncryption(priv, src, encryptionAlias) < 0) + return -1; if (qemuBlockStorageSourceNeedsStorageSliceLayer(src)) src->sliceStorage->nodename = g_strdup_printf("libvirt-%u-slice-sto", src->id); @@ -11163,9 +11173,6 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, qemuDomainPrepareStorageSourceConfig(src, cfg); qemuDomainPrepareDiskSourceData(disk, src); - if (qemuDomainSecretStorageSourcePrepareEncryption(priv, src, nodeformat) < 0) - return -1; - if (!qemuDomainPrepareStorageSourceNbdkit(src, cfg, nodestorage, priv)) { /* If we're using nbdkit to serve the storage source, we don't pass * authentication secrets to qemu, but will pass them to nbdkit instead */ -- 2.42.0
Return the effective storage nodename if the format layer is not present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 7137604e36..b7f16b43ae 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -97,7 +97,10 @@ qemuBlockStorageSourceSetFormatNodename(virStorageSource *src, const char * qemuBlockStorageSourceGetEffectiveNodename(virStorageSource *src) { - return src->nodenameformat; + if (src->nodenameformat) + return src->nodenameformat; + + return qemuBlockStorageSourceGetEffectiveStorageNodename(src); } -- 2.42.0
We want to preserve the wrappers for clarity but the inner logic can be extracted to a common function qemuBlockReopenAccess. In further patches the code from qemuBlockReopenFormat will be merged into the new wrapper as well as logic for handling scenarios with missing 'format' layers will be added. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 62 +++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index b7f16b43ae..1ef6bf98bc 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3221,27 +3221,30 @@ qemuBlockReopenFormat(virDomainObj *vm, /** - * qemuBlockReopenReadWrite: + * qemuBlockReopenAccess: * @vm: domain object * @src: storage source to reopen + * @readonly: requested readonly mode * @asyncJob: qemu async job type * - * Wrapper that reopens @src read-write. We currently depend on qemu - * reopening the storage with 'auto-read-only' enabled for us. - * After successful reopen @src's 'readonly' flag is modified. Does nothing - * if @src is already read-write. + * Reopen @src image to ensure that it is in @readonly. Does nothing if it is + * already in the requested state. + * + * Callers must use qemuBlockReopenReadWrite/qemuBlockReopenReadOnly functions. */ -int -qemuBlockReopenReadWrite(virDomainObj *vm, - virStorageSource *src, - virDomainAsyncJob asyncJob) + +static int +qemuBlockReopenAccess(virDomainObj *vm, + virStorageSource *src, + bool readonly, + virDomainAsyncJob asyncJob) { - if (!src->readonly) + if (src->readonly == readonly) return 0; - src->readonly = false; + src->readonly = readonly; if (qemuBlockReopenFormat(vm, src, asyncJob) < 0) { - src->readonly = true; + src->readonly = !readonly; return -1; } @@ -3250,31 +3253,38 @@ qemuBlockReopenReadWrite(virDomainObj *vm, /** - * qemuBlockReopenReadOnly: + * qemuBlockReopenReadWrite: * @vm: domain object * @src: storage source to reopen * @asyncJob: qemu async job type * - * Wrapper that reopens @src read-only. We currently depend on qemu - * reopening the storage with 'auto-read-only' enabled for us. - * After successful reopen @src's 'readonly' flag is modified. Does nothing - * if @src is already read-only. + * Semantic wrapper that reopens @src read-write. After successful reopen @src's + * 'readonly' flag is modified. Does nothing if @src is already read-write. */ int -qemuBlockReopenReadOnly(virDomainObj *vm, +qemuBlockReopenReadWrite(virDomainObj *vm, virStorageSource *src, virDomainAsyncJob asyncJob) { - if (src->readonly) - return 0; + return qemuBlockReopenAccess(vm, src, false, asyncJob); +} - src->readonly = true; - if (qemuBlockReopenFormat(vm, src, asyncJob) < 0) { - src->readonly = false; - return -1; - } - return 0; +/** + * qemuBlockReopenReadOnly: + * @vm: domain object + * @src: storage source to reopen + * @asyncJob: qemu async job type + * + * Semantic wrapper that reopens @src read-only. After successful reopen @src's + * 'readonly' flag is modified. Does nothing if @src is already read-only. + */ +int +qemuBlockReopenReadOnly(virDomainObj *vm, + virStorageSource *src, + virDomainAsyncJob asyncJob) +{ + return qemuBlockReopenAccess(vm, src, true, asyncJob); } /** -- 2.42.0
Move all the logic into the new function and remove the old one. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 60 +++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 1ef6bf98bc..2872d74fa2 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3182,22 +3182,29 @@ qemuBlockReopenFormatMon(qemuMonitor *mon, /** - * qemuBlockReopenFormat: + * qemuBlockReopenAccess: * @vm: domain object * @src: storage source to reopen + * @readonly: requested readonly mode * @asyncJob: qemu async job type * - * Invokes the 'blockdev-reopen' command on the format layer of @src. This means - * that @src must be already properly configured for the desired outcome. The - * nodenames of @src are used to identify the specific image in qemu. + * Reopen @src image to ensure that it is in @readonly. Does nothing if it is + * already in the requested state. + * + * Callers must use qemuBlockReopenReadWrite/qemuBlockReopenReadOnly functions. */ static int -qemuBlockReopenFormat(virDomainObj *vm, +qemuBlockReopenAccess(virDomainObj *vm, virStorageSource *src, + bool readonly, virDomainAsyncJob asyncJob) { qemuDomainObjPrivate *priv = vm->privateData; int rc; + int ret = -1; + + if (src->readonly == readonly) + return 0; /* If we are lacking the object here, qemu might have opened an image with * a node name unknown to us */ @@ -3207,48 +3214,23 @@ qemuBlockReopenFormat(virDomainObj *vm, return -1; } + src->readonly = readonly; + /* from now on all error paths must use 'goto cleanup' */ + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) - return -1; + goto cleanup; rc = qemuBlockReopenFormatMon(priv->mon, src); qemuDomainObjExitMonitor(vm); if (rc < 0) - return -1; - - return 0; -} - - -/** - * qemuBlockReopenAccess: - * @vm: domain object - * @src: storage source to reopen - * @readonly: requested readonly mode - * @asyncJob: qemu async job type - * - * Reopen @src image to ensure that it is in @readonly. Does nothing if it is - * already in the requested state. - * - * Callers must use qemuBlockReopenReadWrite/qemuBlockReopenReadOnly functions. - */ - -static int -qemuBlockReopenAccess(virDomainObj *vm, - virStorageSource *src, - bool readonly, - virDomainAsyncJob asyncJob) -{ - if (src->readonly == readonly) - return 0; + goto cleanup; - src->readonly = readonly; - if (qemuBlockReopenFormat(vm, src, asyncJob) < 0) { - src->readonly = !readonly; - return -1; - } + ret = 0; - return 0; + cleanup: + src->readonly = !readonly; + return ret; } -- 2.42.0
Take the virJSONValue array object which is passed to the 'blockdev-reopen' command as the 'options' argument rather than making the caller wrap all the properties. The code was a leftover from the time when the blockdev-reopen command had a different syntax, and thus can be cleaned up. Also note that the logging of the node name never worked as the top level object didn't ever contain a 'node-name' property. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 8 +------- src/qemu/qemu_monitor.c | 7 ++----- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 6 ++++-- src/qemu/qemu_monitor_json.h | 2 +- 5 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 2872d74fa2..84d9ddd9ef 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3159,7 +3159,6 @@ int qemuBlockReopenFormatMon(qemuMonitor *mon, virStorageSource *src) { - g_autoptr(virJSONValue) reopenprops = NULL; g_autoptr(virJSONValue) srcprops = NULL; g_autoptr(virJSONValue) reopenoptions = virJSONValueNewArray(); @@ -3169,12 +3168,7 @@ qemuBlockReopenFormatMon(qemuMonitor *mon, if (virJSONValueArrayAppend(reopenoptions, &srcprops) < 0) return -1; - if (virJSONValueObjectAdd(&reopenprops, - "a:options", &reopenoptions, - NULL) < 0) - return -1; - - if (qemuMonitorBlockdevReopen(mon, &reopenprops) < 0) + if (qemuMonitorBlockdevReopen(mon, &reopenoptions) < 0) return -1; return 0; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 320729f067..ec586b9036 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3971,14 +3971,11 @@ qemuMonitorBlockdevAdd(qemuMonitor *mon, int qemuMonitorBlockdevReopen(qemuMonitor *mon, - virJSONValue **props) + virJSONValue **options) { - VIR_DEBUG("props=%p (node-name=%s)", *props, - NULLSTR(virJSONValueObjectGetString(*props, "node-name"))); - QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONBlockdevReopen(mon, props); + return qemuMonitorJSONBlockdevReopen(mon, options); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6c590933aa..c4af9b407d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1309,7 +1309,7 @@ int qemuMonitorBlockdevAdd(qemuMonitor *mon, virJSONValue **props); int qemuMonitorBlockdevReopen(qemuMonitor *mon, - virJSONValue **props); + virJSONValue **options); int qemuMonitorBlockdevDel(qemuMonitor *mon, const char *nodename); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 105d729d7c..9663da4722 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7783,12 +7783,14 @@ qemuMonitorJSONBlockdevAdd(qemuMonitor *mon, int qemuMonitorJSONBlockdevReopen(qemuMonitor *mon, - virJSONValue **props) + virJSONValue **options) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-reopen", props))) + if (!(cmd = qemuMonitorJSONMakeCommand("blockdev-reopen", + "a:options", options, + NULL))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 06023b98ea..ed0027c118 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -691,7 +691,7 @@ qemuMonitorJSONBlockdevAdd(qemuMonitor *mon, int qemuMonitorJSONBlockdevReopen(qemuMonitor *mon, - virJSONValue **props) + virJSONValue **options) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int -- 2.42.0
Use the low level monitor API directly to test the QMP wrapper itself. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 6293b416bd..d9ebb429e7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2594,6 +2594,8 @@ testQemuMonitorJSONBlockdevReopen(const void *opaque) const testGenericData *data = opaque; g_autoptr(qemuMonitorTest) test = NULL; g_autoptr(virStorageSource) src = virStorageSourceNew(); + g_autoptr(virJSONValue) reopenoptions = virJSONValueNewArray(); + g_autoptr(virJSONValue) srcprops = NULL; if (!(test = qemuMonitorTestNewSchema(data->xmlopt, data->schema))) return -1; @@ -2604,10 +2606,16 @@ testQemuMonitorJSONBlockdevReopen(const void *opaque) qemuBlockStorageSourceSetStorageNodename(src, g_strdup("backing nodename")); src->backingStore = virStorageSourceNew(); + if (!(srcprops = qemuBlockStorageSourceGetFormatProps(src, src->backingStore))) + return -1; + + if (virJSONValueArrayAppend(reopenoptions, &srcprops) < 0) + return -1; + if (qemuMonitorTestAddItem(test, "blockdev-reopen", "{\"return\":{}}") < 0) return -1; - if (qemuBlockReopenFormatMon(qemuMonitorTestGetMonitor(test), src) < 0) + if (qemuMonitorBlockdevReopen(qemuMonitorTestGetMonitor(test), &reopenoptions) < 0) return -1; return 0; -- 2.42.0
Move all the code into the now only caller. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 31 +++++++++---------------------- src/qemu/qemu_block.h | 5 ----- 2 files changed, 9 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 84d9ddd9ef..c62f8fe5f3 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3155,26 +3155,6 @@ qemuBlockBitmapsHandleCommitFinish(virStorageSource *topsrc, } -int -qemuBlockReopenFormatMon(qemuMonitor *mon, - virStorageSource *src) -{ - g_autoptr(virJSONValue) srcprops = NULL; - g_autoptr(virJSONValue) reopenoptions = virJSONValueNewArray(); - - if (!(srcprops = qemuBlockStorageSourceGetFormatProps(src, src->backingStore))) - return -1; - - if (virJSONValueArrayAppend(reopenoptions, &srcprops) < 0) - return -1; - - if (qemuMonitorBlockdevReopen(mon, &reopenoptions) < 0) - return -1; - - return 0; -} - - /** * qemuBlockReopenAccess: * @vm: domain object @@ -3193,7 +3173,8 @@ qemuBlockReopenAccess(virDomainObj *vm, bool readonly, virDomainAsyncJob asyncJob) { - qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virJSONValue) reopenoptions = virJSONValueNewArray(); + g_autoptr(virJSONValue) srcprops = NULL; int rc; int ret = -1; @@ -3211,10 +3192,16 @@ qemuBlockReopenAccess(virDomainObj *vm, src->readonly = readonly; /* from now on all error paths must use 'goto cleanup' */ + if (!(srcprops = qemuBlockStorageSourceGetFormatProps(src, src->backingStore))) + return -1; + + if (virJSONValueArrayAppend(reopenoptions, &srcprops) < 0) + return -1; + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) goto cleanup; - rc = qemuBlockReopenFormatMon(priv->mon, src); + rc = qemuMonitorBlockdevReopen(qemuDomainGetMonitor(vm), &reopenoptions); qemuDomainObjExitMonitor(vm); if (rc < 0) diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index dcd8a6ed6c..f37e10216c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -263,11 +263,6 @@ qemuBlockBitmapsHandleCommitFinish(virStorageSource *topsrc, GHashTable *blockNamedNodeData, virJSONValue **actions); -/* only for use in qemumonitorjsontest */ -int -qemuBlockReopenFormatMon(qemuMonitor *mon, - virStorageSource *src); - int qemuBlockReopenReadWrite(virDomainObj *vm, virStorageSource *src, -- 2.42.0
Make the helper reopening a blockdev for access pick the correct layer to reopen based on what is currently in use. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index c62f8fe5f3..0d676f71b0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3192,8 +3192,19 @@ qemuBlockReopenAccess(virDomainObj *vm, src->readonly = readonly; /* from now on all error paths must use 'goto cleanup' */ - if (!(srcprops = qemuBlockStorageSourceGetFormatProps(src, src->backingStore))) - return -1; + /* based on which is the current 'effecitve' layer we must reopen the + * appropriate blockdev */ + if (qemuBlockStorageSourceGetFormatNodename(src)) { + if (!(srcprops = qemuBlockStorageSourceGetFormatProps(src, src->backingStore))) + return -1; + } else if (qemuBlockStorageSourceGetSliceNodename(src)) { + if (!(srcprops = qemuBlockStorageSourceGetBlockdevStorageSliceProps(src, true))) + return -1; + } else { + if (!(srcprops = qemuBlockStorageSourceGetBackendProps(src, + QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_EFFECTIVE_NODE))) + return -1; + } if (virJSONValueArrayAppend(reopenoptions, &srcprops) < 0) return -1; -- 2.42.0
Rewrite the code to use the common tooling for removing blockdevs instead of the ad-hoc qemuBlockStorageSourceDetachOneBlockdev helper. Use of the common infrastructure will properly handle cases when the raw driver is ommited from the block graph. Since the TLS data object is shared for all migration QMP commands and objects we need to strip it's alias from the definition of the storage source before attempting to detach it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index ac58aa1a8c..0f4c6dbe98 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -938,12 +938,26 @@ qemuMigrationSrcNBDCopyCancel(virDomainObj *vm, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDef *disk = vm->def->disks[i]; qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; if (!diskPriv->migrSource) continue; - qemuBlockStorageSourceDetachOneBlockdev(vm, asyncJob, - diskPriv->migrSource); + /* remove the alias of the TLS object when we're about to detach the + * migration NBD blockdev as the TLS object is shared for the migration + * and we don't want to detach it. The alias is not needed after + * the JSON object of the blockdev props is formatted */ + g_clear_pointer(&diskPriv->migrSource->tlsAlias, g_free); + + data = qemuBlockStorageSourceDetachPrepare(diskPriv->migrSource); + + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + goto cleanup; + + qemuBlockStorageSourceAttachRollback(qemuDomainGetMonitor(vm), data); + + qemuDomainObjExitMonitor(vm); + g_clear_pointer(&diskPriv->migrSource, virObjectUnref); } -- 2.42.0
On a Friday in 2023, Peter Krempa wrote:
Rewrite the code to use the common tooling for removing blockdevs instead of the ad-hoc qemuBlockStorageSourceDetachOneBlockdev helper.
Use of the common infrastructure will properly handle cases when the raw driver is ommited from the block graph.
Since the TLS data object is shared for all migration QMP commands and objects we need to strip it's alias from the definition of the storage
*its
source before attempting to detach it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
Jano
The only caller was converted to use the common blockdev infrastructure thus this function is no longer needed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 34 ---------------------------------- src/qemu/qemu_block.h | 5 ----- 2 files changed, 39 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 0d676f71b0..5abbfab67e 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1942,40 +1942,6 @@ qemuBlockStorageSourceChainDetach(qemuMonitor *mon, } -/** - * qemuBlockStorageSourceDetachOneBlockdev: - * @driver: qemu driver object - * @vm: domain object - * @asyncJob: currently running async job - * @src: storage source to detach - * - * Detaches one virStorageSource using blockdev-del. Note that this does not - * detach any authentication/encryption objects. This function enters the - * monitor internally. - */ -int -qemuBlockStorageSourceDetachOneBlockdev(virDomainObj *vm, - virDomainAsyncJob asyncJob, - virStorageSource *src) -{ - int ret; - - if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) - return -1; - - ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm), - qemuBlockStorageSourceGetFormatNodename(src)); - - if (ret == 0) - ret = qemuMonitorBlockdevDel(qemuDomainGetMonitor(vm), - qemuBlockStorageSourceGetStorageNodename(src)); - - qemuDomainObjExitMonitor(vm); - - return ret; -} - - int qemuBlockSnapshotAddBlockdev(virJSONValue *actions, virDomainDiskDef *disk, diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index f37e10216c..0eab0d822c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -149,11 +149,6 @@ void qemuBlockStorageSourceAttachRollback(qemuMonitor *mon, qemuBlockStorageSourceAttachData *data); -int -qemuBlockStorageSourceDetachOneBlockdev(virDomainObj *vm, - virDomainAsyncJob asyncJob, - virStorageSource *src); - struct _qemuBlockStorageSourceChainData { qemuBlockStorageSourceAttachData **srcdata; size_t nsrcdata; -- 2.42.0
The raw driver layer is not needed in this case and can be dropped. Removing the nodename will cause other pieces of the code to pick up and stop addign the layer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0f4c6dbe98..f9c34b72e8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1026,7 +1026,6 @@ qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDef *disk, copysrc->tlsHostname = g_strdup(tlsHostname); qemuBlockStorageSourceSetStorageNodename(copysrc, g_strdup_printf("migration-%s-storage", disk->dst)); - qemuBlockStorageSourceSetFormatNodename(copysrc, g_strdup_printf("migration-%s-format", disk->dst)); return g_steal_pointer(©src); } -- 2.42.0
On a Friday in 2023, Peter Krempa wrote:
The raw driver layer is not needed in this case and can be dropped. Removing the nodename will cause other pieces of the code to pick up and stop addign the layer.
*adding
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 1 - 1 file changed, 1 deletion(-)
Jano
On a Friday in 2023, Peter Krempa wrote:
This series prepares the setup of the block device backend for removal of the raw driver from the block graph, and actually removes it from the migration NBD connection.
Unfortunately for normal usage it's not yet possible as qemu then records the protocol driver name in the backing file format field which would break with older versions of libvirt (see my other series).
Peter Krempa (17): qemu: block: Introduce qemuBlockStorageSourceGetSliceNodename qemu: block: Use qemuBlockStorageSourceNeedsStorageSliceLayer only for setup qemu: block: Introduce helper for deciding when a 'format' layer is needed qemuBlockStorageSourceGetBlockdevStorageSliceProps: Allow turning the slice layer into effective blockdev layer qemuBlockStorageSourceAttachPrepareBlockdev: Prepare for optionally missing format layer qemuBlockStorageSourceDetachPrepare: Prepare for possibly missing 'format' layer qemuDomainPrepareStorageSourceBlockdevNodename: Restructure code to allow missing 'format' layer qemuBlockStorageSourceGetEffectiveNodename: Prepare for missing 'format' driver qemu: block: Extract logic from qemuBlockReopenReadWrite/ReadOnly qemu: block: Absorb logic from qemuBlockReopenFormat to qemuBlockReopenAccess qemu: monitor: Sanitize arguments of qemuMonitorBlockdevReopen testQemuMonitorJSONBlockdevReopen: Don't use qemuBlockReopenFormatMon qemu: block: Absorb qemuBlockReopenFormatMon into qemuBlockReopenAccess qemuBlockReopenAccess: prepare for removal of 'raw' format layer qemuMigrationSrcNBDCopyCancel: Use qemuBlockStorageSourceAttachRollback to detach migration NBD blockdevs qemu: block: Remove unused qemuBlockStorageSourceDetachOneBlockdev qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource: Don't setup 'raw' layer for migration NBD connection
src/qemu/qemu_block.c | 257 ++++++++++++++++++----------------- src/qemu/qemu_block.h | 16 +-- src/qemu/qemu_domain.c | 17 ++- src/qemu/qemu_migration.c | 19 ++- src/qemu/qemu_monitor.c | 7 +- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 6 +- src/qemu/qemu_monitor_json.h | 2 +- tests/qemumonitorjsontest.c | 10 +- 9 files changed, 185 insertions(+), 151 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa