[PATCH 00/35] qemu: Implement support for transient disk hotplug and backing store sharing
This series refactors the qemu disk hotplug code so that it can be easily extended for hotplug of transient disks and then also used at startup to plug in disks where the users want to share the backing image. Masayoshi Mizuma (1): qemu_snapshot: Add the guest name to the transient disk path Peter Krempa (34): qemu: snapshot: Extract setup of snapshot disk definition for transient disks qemu: process: Setup transient disks only when starting a fresh VM qemuSnapshotDiskPrepareOne: Pass in qemuSnapshotDiskContext qemuSnapshotDiskContext: Store virQEMUDriverConfig in the struct qemuSnapshotDiskPrepareOne: Use data from qemuSnapshotDiskContext qemuSnapshotDiskCreate: Use 'cfg' from the qemuSnapshotDiskContext qemu: snapshot: move transient snapshot code to qemu_process qemu: Move 'bootindex' handling for disks out of command line formatter qemu: Move bootindex usage logic into qemuBuildDiskDeviceStr qemuxml2argvtest: Remove pointless tests for keywrapping on s390 qemu: Move iothread and s390 address validation for disk devices into the validator Replace virDomainDiskInsertPreAlloced by virDomainDiskInsert conf: remove virDomainDiskInsertPreAlloced qemuBlockStorageSourceChainData: Add handling of 'copy-on-read' filter layer qemuDomainAttachDiskGeneric: Move 'copy-on-read' handling to qemuBlockStorageSourceChainData qemuDomainRemoveDiskDevice: Move 'copy-on-read' handling to qemuBlockStorageSourceChainData qemuDomainAttachDeviceDiskLiveInternal: Absorb qemuDomainAttachUSBMassStorageDevice qemuDomainAttachDeviceDiskLiveInternal: Absorb qemuDomainAttachVirtioDiskDevice qemuDomainAttachDeviceDiskLiveInternal: Absorb qemuDomainAttachSCSIDisk qemuDomainAttachDeviceDiskLiveInternal: Simplify call to qemuDomainAttachDiskGeneric qemuDomainAttachDiskGeneric: Move setup of disk into qemuDomainAttachDeviceDiskLiveInternal qemu: hotplug: Move post-insertion steps of disk hotplug to qemuDomainAttachDeviceDiskLiveInternal qemuDomainAttachDiskGeneric: Fix whitespace qemuDomainAttachDiskGeneric: Refactor cleanup qemuDomainAttachDiskGeneric: Move PR helper attach into qemuDomainAttachDeviceDiskLiveInternal qemuDomainAttachDiskGeneric: Refactor rollback handling qemuDomainAttachDiskGeneric: Split up frontend and backend attachment qemu: Track creation of <transient> disk overlay individually qemuDomainAttachDiskGeneric: Implement hotplug of <transient> disk qemuDomainAttachDiskGeneric: Pass the qemu async job type qemuDomainAttachDiskGeneric: Export conf: Introduce 'shareBacking' for <transient> disks qemu: Allow <transient> disks with images shared accross VMs tests: Add qemuxml2argv and qemuxml2xml test for <transient shareBacking='yes'> docs/formatdomain.rst | 6 + docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c | 33 +- src/conf/domain_conf.h | 3 +- src/libvirt_private.syms | 1 - src/libxl/libxl_driver.c | 4 +- src/lxc/lxc_driver.c | 6 +- src/qemu/qemu_block.c | 12 + src/qemu/qemu_block.h | 4 + src/qemu/qemu_command.c | 134 ++----- src/qemu/qemu_command.h | 1 - src/qemu/qemu_domain.c | 2 - src/qemu/qemu_domain.h | 11 +- src/qemu/qemu_hotplug.c | 335 +++++++++--------- src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_process.c | 203 ++++++++++- src/qemu/qemu_snapshot.c | 160 +++------ src/qemu/qemu_snapshot.h | 27 +- src/qemu/qemu_validate.c | 73 ++++ .../disk-transient.x86_64-4.1.0.err | 2 +- .../disk-transient.x86_64-latest.args | 4 +- tests/qemuxml2argvdata/disk-transient.xml | 6 + tests/qemuxml2argvtest.c | 8 - .../disk-transient.x86_64-latest.xml | 48 +++ tests/qemuxml2xmltest.c | 3 +- 25 files changed, 644 insertions(+), 453 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/disk-transient.x86_64-latest.xml -- 2.31.1
The code will be later reused when adding support for sharing the backing image of the snapshot. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index fd6669433b..c3cff46bc9 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1168,6 +1168,29 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, } +static virDomainSnapshotDiskDef * +qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) +{ + g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1); + + snapdisk->name = g_strdup(domdisk->dst); + snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + snapdisk->src = virStorageSourceNew(); + snapdisk->src->type = VIR_STORAGE_TYPE_FILE; + snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; + snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path); + + if (virFileExists(snapdisk->src->path)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Overlay file '%s' for transient disk '%s' already exists"), + snapdisk->src->path, domdisk->dst); + return NULL; + } + + return g_steal_pointer(&snapdisk); +} + + static qemuSnapshotDiskContext * qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, virQEMUDriverConfig *cfg, @@ -1181,26 +1204,15 @@ qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDef *domdisk = vm->def->disks[i]; - g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1); + g_autoptr(virDomainSnapshotDiskDef) snapdisk = NULL; if (!domdisk->transient) continue; /* validation code makes sure that we do this only for local disks * with a file source */ - snapdisk->name = g_strdup(domdisk->dst); - snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - snapdisk->src = virStorageSourceNew(); - snapdisk->src->type = VIR_STORAGE_TYPE_FILE; - snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; - snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path); - - if (virFileExists(snapdisk->src->path)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Overlay file '%s' for transient disk '%s' already exists"), - snapdisk->src->path, domdisk->dst); - return NULL; - } + + snapdisk = qemuSnapshotGetTransientDiskDef(domdisk); if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk, snapctxt->dd + snapctxt->ndd++, -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
The code will be later reused when adding support for sharing the backing image of the snapshot.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index fd6669433b..c3cff46bc9 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1168,6 +1168,29 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, }
+static virDomainSnapshotDiskDef * +qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) +{ + g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1); + + snapdisk->name = g_strdup(domdisk->dst); + snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + snapdisk->src = virStorageSourceNew(); + snapdisk->src->type = VIR_STORAGE_TYPE_FILE; + snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; + snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path); + + if (virFileExists(snapdisk->src->path)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Overlay file '%s' for transient disk '%s' already exists"), + snapdisk->src->path, domdisk->dst); + return NULL; + } + + return g_steal_pointer(&snapdisk); +} + + static qemuSnapshotDiskContext * qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, virQEMUDriverConfig *cfg, @@ -1181,26 +1204,15 @@ qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm,
for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDef *domdisk = vm->def->disks[i]; - g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1); + g_autoptr(virDomainSnapshotDiskDef) snapdisk = NULL;
if (!domdisk->transient) continue;
/* validation code makes sure that we do this only for local disks * with a file source */ - snapdisk->name = g_strdup(domdisk->dst); - snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - snapdisk->src = virStorageSourceNew(); - snapdisk->src->type = VIR_STORAGE_TYPE_FILE; - snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; - snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path); - - if (virFileExists(snapdisk->src->path)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Overlay file '%s' for transient disk '%s' already exists"), - snapdisk->src->path, domdisk->dst); - return NULL; - } + + snapdisk = qemuSnapshotGetTransientDiskDef(domdisk);
if (!snapdisk) return NULL; To correctly propagate the error and avoid NULL dereference below Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk, snapctxt->dd + snapctxt->ndd++, -- 2.31.1
Creating the overlay for the disk is needed when starting a new VM only. Additionally for now migration with transient disks is forbidden anyways. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 35213f81ec..543b50f875 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7289,9 +7289,11 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessAutoDestroyAdd(driver, vm, conn) < 0) goto cleanup; - VIR_DEBUG("Setting up transient disk"); - if (qemuSnapshotCreateDisksTransient(vm, asyncJob) < 0) - goto cleanup; + if (!incoming && !snapshot) { + VIR_DEBUG("Setting up transient disk"); + if (qemuSnapshotCreateDisksTransient(vm, asyncJob) < 0) + goto cleanup; + } ret = 0; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Creating the overlay for the disk is needed when starting a new VM only. Additionally for now migration with transient disks is forbidden anyways.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Rather than filling various parts of the context from arguments pass in the whole context. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c3cff46bc9..b93b9b9fb0 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1030,12 +1030,11 @@ qemuSnapshotDiskPrepareOne(virDomainObj *vm, virQEMUDriverConfig *cfg, virDomainDiskDef *disk, virDomainSnapshotDiskDef *snapdisk, - qemuSnapshotDiskData *dd, + qemuSnapshotDiskContext *snapctxt, GHashTable *blockNamedNodeData, bool reuse, bool updateConfig, - qemuDomainAsyncJob asyncJob, - virJSONValue *actions) + qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; @@ -1043,6 +1042,7 @@ qemuSnapshotDiskPrepareOne(virDomainObj *vm, virDomainDiskDef *persistdisk; bool supportsCreate; bool updateRelativeBacking = false; + qemuSnapshotDiskData *dd = snapctxt->dd + snapctxt->ndd++; dd->disk = disk; @@ -1115,13 +1115,13 @@ qemuSnapshotDiskPrepareOne(virDomainObj *vm, blockNamedNodeData, asyncJob) < 0) return -1; - if (qemuSnapshotDiskBitmapsPropagate(dd, actions, blockNamedNodeData) < 0) + if (qemuSnapshotDiskBitmapsPropagate(dd, snapctxt->actions, blockNamedNodeData) < 0) return -1; - if (qemuBlockSnapshotAddBlockdev(actions, dd->disk, dd->src) < 0) + if (qemuBlockSnapshotAddBlockdev(snapctxt->actions, dd->disk, dd->src) < 0) return -1; } else { - if (qemuBlockSnapshotAddLegacy(actions, dd->disk, dd->src, reuse) < 0) + if (qemuBlockSnapshotAddLegacy(snapctxt->actions, dd->disk, dd->src, reuse) < 0) return -1; } @@ -1155,12 +1155,11 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, if (qemuSnapshotDiskPrepareOne(vm, cfg, vm->def->disks[i], snapdef->disks + i, - snapctxt->dd + snapctxt->ndd++, + snapctxt, blockNamedNodeData, reuse, true, - asyncJob, - snapctxt->actions) < 0) + asyncJob) < 0) return NULL; } @@ -1214,13 +1213,11 @@ qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, snapdisk = qemuSnapshotGetTransientDiskDef(domdisk); - if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk, - snapctxt->dd + snapctxt->ndd++, + if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk, snapctxt, blockNamedNodeData, false, false, - asyncJob, - snapctxt->actions) < 0) + asyncJob) < 0) return NULL; } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Rather than filling various parts of the context from arguments pass in the whole context.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The config is used both with the preparation and execution functions, so we can store it in the context to simplify other helpers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b93b9b9fb0..def2733958 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -896,6 +896,8 @@ struct _qemuSnapshotDiskContext { virJSONValue *actions; + virQEMUDriverConfig *cfg; + /* needed for automatic cleanup of 'dd' */ virDomainObj *vm; qemuDomainAsyncJob asyncJob; @@ -909,11 +911,14 @@ qemuSnapshotDiskContextNew(size_t ndisks, virDomainObj *vm, qemuDomainAsyncJob asyncJob) { + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; qemuSnapshotDiskContext *ret = g_new0(qemuSnapshotDiskContext, 1); ret->dd = g_new0(qemuSnapshotDiskData, ndisks); ret->actions = virJSONValueNewArray(); ret->vm = vm; + ret->cfg = virQEMUDriverGetConfig(driver); ret->asyncJob = asyncJob; return ret; @@ -930,6 +935,8 @@ qemuSnapshotDiskContextCleanup(qemuSnapshotDiskContext *snapctxt) qemuSnapshotDiskCleanup(snapctxt->dd, snapctxt->ndd, snapctxt->vm, snapctxt->asyncJob); + virObjectUnref(snapctxt->cfg); + g_free(snapctxt); } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
The config is used both with the preparation and execution functions, so we can store it in the context to simplify other helpers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 +++++++ 1 file changed, 7 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove all the arguments which are present in qemuSnapshotDiskContext. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index def2733958..0a93621b75 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1033,16 +1033,14 @@ qemuSnapshotDiskPrepareOneBlockdev(virQEMUDriver *driver, static int -qemuSnapshotDiskPrepareOne(virDomainObj *vm, - virQEMUDriverConfig *cfg, +qemuSnapshotDiskPrepareOne(qemuSnapshotDiskContext *snapctxt, virDomainDiskDef *disk, virDomainSnapshotDiskDef *snapdisk, - qemuSnapshotDiskContext *snapctxt, GHashTable *blockNamedNodeData, bool reuse, - bool updateConfig, - qemuDomainAsyncJob asyncJob) + bool updateConfig) { + virDomainObj *vm = snapctxt->vm; qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); @@ -1118,8 +1116,8 @@ qemuSnapshotDiskPrepareOne(virDomainObj *vm, dd->prepared = true; if (blockdev) { - if (qemuSnapshotDiskPrepareOneBlockdev(driver, vm, dd, cfg, reuse, - blockNamedNodeData, asyncJob) < 0) + if (qemuSnapshotDiskPrepareOneBlockdev(driver, vm, dd, snapctxt->cfg, reuse, + blockNamedNodeData, snapctxt->asyncJob) < 0) return -1; if (qemuSnapshotDiskBitmapsPropagate(dd, snapctxt->actions, blockNamedNodeData) < 0) @@ -1145,7 +1143,6 @@ qemuSnapshotDiskPrepareOne(virDomainObj *vm, static qemuSnapshotDiskContext * qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, virDomainMomentObj *snap, - virQEMUDriverConfig *cfg, bool reuse, GHashTable *blockNamedNodeData, qemuDomainAsyncJob asyncJob) @@ -1160,13 +1157,12 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, if (snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; - if (qemuSnapshotDiskPrepareOne(vm, cfg, vm->def->disks[i], + if (qemuSnapshotDiskPrepareOne(snapctxt, + vm->def->disks[i], snapdef->disks + i, - snapctxt, blockNamedNodeData, reuse, - true, - asyncJob) < 0) + true) < 0) return NULL; } @@ -1199,7 +1195,6 @@ qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) static qemuSnapshotDiskContext * qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, - virQEMUDriverConfig *cfg, GHashTable *blockNamedNodeData, qemuDomainAsyncJob asyncJob) { @@ -1220,11 +1215,10 @@ qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, snapdisk = qemuSnapshotGetTransientDiskDef(domdisk); - if (qemuSnapshotDiskPrepareOne(vm, cfg, domdisk, snapdisk, snapctxt, + if (qemuSnapshotDiskPrepareOne(snapctxt, domdisk, snapdisk, blockNamedNodeData, false, - false, - asyncJob) < 0) + false) < 0) return NULL; } @@ -1349,7 +1343,7 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm, /* prepare a list of objects to use in the vm definition so that we don't * have to roll back later */ - if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, cfg, reuse, + if (!(snapctxt = qemuSnapshotDiskPrepareActiveExternal(vm, snap, reuse, blockNamedNodeData, asyncJob))) return -1; @@ -1383,7 +1377,7 @@ qemuSnapshotCreateDisksTransient(virDomainObj *vm, if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) return -1; - if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, cfg, + if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, blockNamedNodeData, asyncJob))) return -1; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Remove all the arguments which are present in qemuSnapshotDiskContext.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We store the virQEMUDriverConfig object in the context. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 0a93621b75..8855a019cb 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1285,8 +1285,7 @@ qemuSnapshotDiskUpdateSource(virDomainObj *vm, static int -qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt, - virQEMUDriverConfig *cfg) +qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt) { qemuDomainObjPrivate *priv = snapctxt->vm->privateData; virQEMUDriver *driver = priv->driver; @@ -1317,9 +1316,9 @@ qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt, if (rc < 0) return -1; - if (virDomainObjSave(snapctxt->vm, driver->xmlopt, cfg->stateDir) < 0 || + if (virDomainObjSave(snapctxt->vm, driver->xmlopt, snapctxt->cfg->stateDir) < 0 || (snapctxt->vm->newDef && virDomainDefSave(snapctxt->vm->newDef, driver->xmlopt, - cfg->configDir) < 0)) + snapctxt->cfg->configDir) < 0)) return -1; return 0; @@ -1332,7 +1331,6 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm, virDomainMomentObj *snap, GHashTable *blockNamedNodeData, unsigned int flags, - virQEMUDriverConfig *cfg, qemuDomainAsyncJob asyncJob) { bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; @@ -1347,7 +1345,7 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm, blockNamedNodeData, asyncJob))) return -1; - if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0) + if (qemuSnapshotDiskCreate(snapctxt) < 0) return -1; return 0; @@ -1368,8 +1366,6 @@ qemuSnapshotCreateDisksTransient(virDomainObj *vm, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivate *priv = vm->privateData; - virQEMUDriver *driver = priv->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; @@ -1382,7 +1378,7 @@ qemuSnapshotCreateDisksTransient(virDomainObj *vm, asyncJob))) return -1; - if (qemuSnapshotDiskCreate(snapctxt, cfg) < 0) + if (qemuSnapshotDiskCreate(snapctxt) < 0) return -1; } @@ -1518,7 +1514,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, /* the domain is now paused if a memory snapshot was requested */ if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap, - blockNamedNodeData, flags, cfg, + blockNamedNodeData, flags, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) goto cleanup; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
We store the virQEMUDriverConfig object in the context.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The code deals with the startup of the VM and just uses the snapshot code to achieve the desired outcome. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 61 +++++++++++++++++++++++++++++- src/qemu/qemu_snapshot.c | 82 +++------------------------------------- src/qemu/qemu_snapshot.h | 26 ++++++++++++- 3 files changed, 89 insertions(+), 80 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 543b50f875..9222f16caa 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6937,6 +6937,65 @@ qemuProcessEnablePerf(virDomainObj *vm) } +static int +qemuProcessSetupDisksTransientSnapshot(virDomainObj *vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; + g_autoptr(GHashTable) blockNamedNodeData = NULL; + size_t i; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) + return -1; + + snapctxt = qemuSnapshotDiskContextNew(vm->def->ndisks, vm, asyncJob); + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *domdisk = vm->def->disks[i]; + g_autoptr(virDomainSnapshotDiskDef) snapdisk = NULL; + + if (!domdisk->transient) + continue; + + /* validation code makes sure that we do this only for local disks + * with a file source */ + + snapdisk = qemuSnapshotGetTransientDiskDef(domdisk); + + if (qemuSnapshotDiskPrepareOne(snapctxt, domdisk, snapdisk, + blockNamedNodeData, + false, + false) < 0) + return -1; + } + + if (qemuSnapshotDiskCreate(snapctxt) < 0) + return -1; + + /* the overlays are established, so they can be deleted on shutdown */ + priv->inhibitDiskTransientDelete = false; + + return 0; +} + + +static int +qemuProcessSetupDisksTransient(virDomainObj *vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivate *priv = vm->privateData; + + if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))) + return 0; + + if (qemuProcessSetupDisksTransientSnapshot(vm, asyncJob) < 0) + return -1; + + return 0; +} + + /** * qemuProcessLaunch: * @@ -7291,7 +7350,7 @@ qemuProcessLaunch(virConnectPtr conn, if (!incoming && !snapshot) { VIR_DEBUG("Setting up transient disk"); - if (qemuSnapshotCreateDisksTransient(vm, asyncJob) < 0) + if (qemuProcessSetupDisksTransient(vm, asyncJob) < 0) goto cleanup; } diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8855a019cb..928b7af287 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -906,7 +906,7 @@ struct _qemuSnapshotDiskContext { typedef struct _qemuSnapshotDiskContext qemuSnapshotDiskContext; -static qemuSnapshotDiskContext * +qemuSnapshotDiskContext * qemuSnapshotDiskContextNew(size_t ndisks, virDomainObj *vm, qemuDomainAsyncJob asyncJob) @@ -925,7 +925,7 @@ qemuSnapshotDiskContextNew(size_t ndisks, } -static void +void qemuSnapshotDiskContextCleanup(qemuSnapshotDiskContext *snapctxt) { if (!snapctxt) @@ -940,8 +940,6 @@ qemuSnapshotDiskContextCleanup(qemuSnapshotDiskContext *snapctxt) g_free(snapctxt); } -G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuSnapshotDiskContext, qemuSnapshotDiskContextCleanup); - /** * qemuSnapshotDiskBitmapsPropagate: @@ -1032,7 +1030,7 @@ qemuSnapshotDiskPrepareOneBlockdev(virQEMUDriver *driver, } -static int +int qemuSnapshotDiskPrepareOne(qemuSnapshotDiskContext *snapctxt, virDomainDiskDef *disk, virDomainSnapshotDiskDef *snapdisk, @@ -1170,7 +1168,7 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, } -static virDomainSnapshotDiskDef * +virDomainSnapshotDiskDef * qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) { g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1); @@ -1193,39 +1191,6 @@ qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) } -static qemuSnapshotDiskContext * -qemuSnapshotDiskPrepareDisksTransient(virDomainObj *vm, - GHashTable *blockNamedNodeData, - qemuDomainAsyncJob asyncJob) -{ - g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; - size_t i; - - snapctxt = qemuSnapshotDiskContextNew(vm->def->ndisks, vm, asyncJob); - - for (i = 0; i < vm->def->ndisks; i++) { - virDomainDiskDef *domdisk = vm->def->disks[i]; - g_autoptr(virDomainSnapshotDiskDef) snapdisk = NULL; - - if (!domdisk->transient) - continue; - - /* validation code makes sure that we do this only for local disks - * with a file source */ - - snapdisk = qemuSnapshotGetTransientDiskDef(domdisk); - - if (qemuSnapshotDiskPrepareOne(snapctxt, domdisk, snapdisk, - blockNamedNodeData, - false, - false) < 0) - return NULL; - } - - return g_steal_pointer(&snapctxt); -} - - static void qemuSnapshotDiskUpdateSourceRenumber(virStorageSource *src) { @@ -1284,7 +1249,7 @@ qemuSnapshotDiskUpdateSource(virDomainObj *vm, } -static int +int qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt) { qemuDomainObjPrivate *priv = snapctxt->vm->privateData; @@ -1352,43 +1317,6 @@ qemuSnapshotCreateActiveExternalDisks(virDomainObj *vm, } -/** - * qemuSnapshotCreateDisksTransient: - * @vm: domain object - * @asyncJob: qemu async job type - * - * Creates overlays on top of disks which are configured as <transient/>. Note - * that the validation code ensures that <transient> disks have appropriate - * configuration. - */ -int -qemuSnapshotCreateDisksTransient(virDomainObj *vm, - qemuDomainAsyncJob asyncJob) -{ - qemuDomainObjPrivate *priv = vm->privateData; - g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; - g_autoptr(GHashTable) blockNamedNodeData = NULL; - - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { - if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) - return -1; - - if (!(snapctxt = qemuSnapshotDiskPrepareDisksTransient(vm, - blockNamedNodeData, - asyncJob))) - return -1; - - if (qemuSnapshotDiskCreate(snapctxt) < 0) - return -1; - } - - /* the overlays are established, so they can be deleted on shutdown */ - priv->inhibitDiskTransientDelete = false; - - return 0; -} - - static int qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h index 62ff22221d..4fba7e4e24 100644 --- a/src/qemu/qemu_snapshot.h +++ b/src/qemu/qemu_snapshot.h @@ -55,6 +55,28 @@ qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, unsigned int flags); + +typedef struct _qemuSnapshotDiskContext qemuSnapshotDiskContext; + +qemuSnapshotDiskContext * +qemuSnapshotDiskContextNew(size_t ndisks, + virDomainObj *vm, + qemuDomainAsyncJob asyncJob); + +void +qemuSnapshotDiskContextCleanup(qemuSnapshotDiskContext *snapctxt); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuSnapshotDiskContext, qemuSnapshotDiskContextCleanup); + +int +qemuSnapshotDiskPrepareOne(qemuSnapshotDiskContext *snapctxt, + virDomainDiskDef *disk, + virDomainSnapshotDiskDef *snapdisk, + GHashTable *blockNamedNodeData, + bool reuse, + bool updateConfig); int -qemuSnapshotCreateDisksTransient(virDomainObj *vm, - qemuDomainAsyncJob asyncJob); +qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt); + +virDomainSnapshotDiskDef * +qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
The code deals with the startup of the VM and just uses the snapshot code to achieve the desired outcome.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 61 +++++++++++++++++++++++++++++- src/qemu/qemu_snapshot.c | 82 +++------------------------------------- src/qemu/qemu_snapshot.h | 26 ++++++++++++- 3 files changed, 89 insertions(+), 80 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Later patches will implement sharing of the backing file, so we'll need to be able to discriminate the overlays per VM. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_snapshot.c | 6 ++++-- src/qemu/qemu_snapshot.h | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9222f16caa..d743a581b3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6961,7 +6961,7 @@ qemuProcessSetupDisksTransientSnapshot(virDomainObj *vm, /* validation code makes sure that we do this only for local disks * with a file source */ - snapdisk = qemuSnapshotGetTransientDiskDef(domdisk); + snapdisk = qemuSnapshotGetTransientDiskDef(domdisk, vm->def->name); if (qemuSnapshotDiskPrepareOne(snapctxt, domdisk, snapdisk, blockNamedNodeData, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 928b7af287..29e86342d6 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1169,7 +1169,8 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, virDomainSnapshotDiskDef * -qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) +qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk, + const char *suffix) { g_autoptr(virDomainSnapshotDiskDef) snapdisk = g_new0(virDomainSnapshotDiskDef, 1); @@ -1178,7 +1179,8 @@ qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk) snapdisk->src = virStorageSourceNew(); snapdisk->src->type = VIR_STORAGE_TYPE_FILE; snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; - snapdisk->src->path = g_strdup_printf("%s.TRANSIENT", domdisk->src->path); + snapdisk->src->path = g_strdup_printf("%s.TRANSIENT-%s", + domdisk->src->path, suffix); if (virFileExists(snapdisk->src->path)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h index 4fba7e4e24..ad2bdb1114 100644 --- a/src/qemu/qemu_snapshot.h +++ b/src/qemu/qemu_snapshot.h @@ -79,4 +79,5 @@ int qemuSnapshotDiskCreate(qemuSnapshotDiskContext *snapctxt); virDomainSnapshotDiskDef * -qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk); +qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk, + const char *suffix); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Later patches will implement sharing of the backing file, so we'll need to be able to discriminate the overlays per VM.
Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_snapshot.c | 6 ++++-- src/qemu/qemu_snapshot.h | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The logic assigning the bootindices from the legacy boot orded configuration was spread through the command line formatters for the disk device and for the floppy controller. This patch adds 'effectiveBootindex' property to the disk private data which holds the calculated boot index and moves the logic of determining the boot index into 'qemuProcessPrepareDomainDiskBootorder' called from 'qemuProcessPrepareDomainStorage'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 63 +++++++-------------------------------- src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_process.c | 66 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dcc060bde9..b01421f61b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1976,13 +1976,11 @@ qemuCommandAddExtDevice(virCommand *cmd, static int qemuBuildFloppyCommandLineControllerOptions(virCommand *cmd, const virDomainDef *def, - virQEMUCaps *qemuCaps, - unsigned int bootFloppy) + virQEMUCaps *qemuCaps) { g_auto(virBuffer) fdc_opts = VIR_BUFFER_INITIALIZER; bool explicitfdc = qemuDomainNeedsFDC(def); bool hasfloppy = false; - unsigned int bootindex; char driveLetter; size_t i; @@ -1993,26 +1991,21 @@ qemuBuildFloppyCommandLineControllerOptions(virCommand *cmd, g_autofree char *backendStr = NULL; g_autofree char *bootindexStr = NULL; virDomainDiskDef *disk = def->disks[i]; + qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); if (disk->bus != VIR_DOMAIN_DISK_BUS_FDC) continue; hasfloppy = true; - if (disk->info.bootIndex) { - bootindex = disk->info.bootIndex; - } else { - bootindex = bootFloppy; - bootFloppy = 0; - } - if (disk->info.addr.drive.unit) driveLetter = 'B'; else driveLetter = 'A'; - if (bootindex) - bootindexStr = g_strdup_printf("bootindex%c=%u", driveLetter, bootindex); + if (diskPriv->effectiveBootindex > 0) + bootindexStr = g_strdup_printf("bootindex%c=%u", driveLetter, + diskPriv->effectiveBootindex); /* with -blockdev we setup the floppy device and it's backend with -device */ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) { @@ -2210,66 +2203,30 @@ qemuBuildDisksCommandLine(virCommand *cmd, virQEMUCaps *qemuCaps) { size_t i; - unsigned int bootCD = 0; - unsigned int bootFloppy = 0; - unsigned int bootDisk = 0; bool blockdev = virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV); - for (i = 0; i < def->os.nBootDevs; i++) { - switch (def->os.bootDevs[i]) { - case VIR_DOMAIN_BOOT_CDROM: - bootCD = i + 1; - break; - case VIR_DOMAIN_BOOT_FLOPPY: - bootFloppy = i + 1; - break; - case VIR_DOMAIN_BOOT_DISK: - bootDisk = i + 1; - break; - } - } - /* If we want to express the floppy drives via -device, the controller needs * to be instantiated prior to that */ if (blockdev && - qemuBuildFloppyCommandLineControllerOptions(cmd, def, qemuCaps, bootFloppy) < 0) + qemuBuildFloppyCommandLineControllerOptions(cmd, def, qemuCaps) < 0) return -1; for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; + qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); unsigned int bootindex = 0; - if (disk->info.bootIndex) { - bootindex = disk->info.bootIndex; - } else { - switch (disk->device) { - case VIR_DOMAIN_DISK_DEVICE_CDROM: - bootindex = bootCD; - bootCD = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_DISK: - case VIR_DOMAIN_DISK_DEVICE_LUN: - bootindex = bootDisk; - bootDisk = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - case VIR_DOMAIN_DISK_DEVICE_LAST: - default: - break; - } - } - /* The floppy device itself does not support the bootindex property * so we need to set it up for the controller */ - if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) - bootindex = 0; + if (disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) + bootindex = diskPriv->effectiveBootindex; if (qemuBuildDiskCommandLine(cmd, def, disk, qemuCaps, bootindex) < 0) return -1; } if (!blockdev && - qemuBuildFloppyCommandLineControllerOptions(cmd, def, qemuCaps, bootFloppy) < 0) + qemuBuildFloppyCommandLineControllerOptions(cmd, def, qemuCaps) < 0) return -1; return 0; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2626f5dcaa..4fc1969d9e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -288,6 +288,10 @@ struct _qemuDomainDiskPrivate { char *qomName; /* QOM path of the disk (also refers to the block backend) */ char *nodeCopyOnRead; /* nodename of the disk-wide copy-on-read blockdev layer */ + + unsigned int effectiveBootindex; /* boot index of the disk based on one + of the two ways we use to select a boot + device */ }; #define QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d743a581b3..7fe051505b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6290,6 +6290,70 @@ qemuProcessPrepareDomainNUMAPlacement(virDomainObj *vm) } +static void +qemuProcessPrepareDomainDiskBootorder(virDomainDef *def) +{ + size_t i; + unsigned int bootCD = 0; + unsigned int bootFloppy = 0; + unsigned int bootDisk = 0; + + for (i = 0; i < def->os.nBootDevs; i++) { + switch ((virDomainBootOrder) def->os.bootDevs[i]) { + case VIR_DOMAIN_BOOT_CDROM: + bootCD = i + 1; + break; + + case VIR_DOMAIN_BOOT_FLOPPY: + bootFloppy = i + 1; + break; + + case VIR_DOMAIN_BOOT_DISK: + bootDisk = i + 1; + break; + + case VIR_DOMAIN_BOOT_NET: + /* network boot is handled in network device formatting code */ + case VIR_DOMAIN_BOOT_LAST: + default: + break; + } + } + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDef *disk = def->disks[i]; + qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + + if (disk->info.bootIndex > 0) { + diskPriv->effectiveBootindex = disk->info.bootIndex; + continue; + } + + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + diskPriv->effectiveBootindex = bootCD; + bootCD = 0; + break; + + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + diskPriv->effectiveBootindex = bootDisk; + bootDisk = 0; + break; + + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + diskPriv->effectiveBootindex = bootFloppy; + bootFloppy = 0; + break; + + case VIR_DOMAIN_DISK_DEVICE_LAST: + default: + break; + } + } +} + + static int qemuProcessPrepareDomainStorage(virQEMUDriver *driver, virDomainObj *vm, @@ -6316,6 +6380,8 @@ qemuProcessPrepareDomainStorage(virQEMUDriver *driver, return -1; } + qemuProcessPrepareDomainDiskBootorder(vm->def); + return 0; } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
The logic assigning the bootindices from the legacy boot orded
s/orded/order/
configuration was spread through the command line formatters for the disk device and for the floppy controller.
This patch adds 'effectiveBootindex' property to the disk private data which holds the calculated boot index and moves the logic of determining the boot index into 'qemuProcessPrepareDomainDiskBootorder' called from 'qemuProcessPrepareDomainStorage'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 63 +++++++-------------------------------- src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_process.c | 66 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 53 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We can skip the formatting of the bootindex for floppies directly at the place where it's being formatted. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 23 ++++++++--------------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b01421f61b..200f9a04b1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1659,9 +1659,9 @@ qemuBuildDriveDevCacheStr(virDomainDiskDef *disk, char * qemuBuildDiskDeviceStr(const virDomainDef *def, virDomainDiskDef *disk, - unsigned int bootindex, virQEMUCaps *qemuCaps) { + qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); g_auto(virBuffer) opt = VIR_BUFFER_INITIALIZER; const char *contAlias; g_autofree char *backendAlias = NULL; @@ -1876,8 +1876,10 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, } virBufferAsprintf(&opt, ",id=%s", disk->info.alias); - if (bootindex) - virBufferAsprintf(&opt, ",bootindex=%u", bootindex); + /* bootindex for floppies is configured via the fdc controller */ + if (disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + diskPriv->effectiveBootindex > 0) + virBufferAsprintf(&opt, ",bootindex=%u", diskPriv->effectiveBootindex); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKIO)) { if (disk->blockio.logical_block_size > 0) virBufferAsprintf(&opt, ",logical_block_size=%u", @@ -2164,8 +2166,7 @@ static int qemuBuildDiskCommandLine(virCommand *cmd, const virDomainDef *def, virDomainDiskDef *disk, - virQEMUCaps *qemuCaps, - unsigned int bootindex) + virQEMUCaps *qemuCaps) { g_autofree char *optstr = NULL; @@ -2188,8 +2189,7 @@ qemuBuildDiskCommandLine(virCommand *cmd, virCommandAddArg(cmd, "-device"); - if (!(optstr = qemuBuildDiskDeviceStr(def, disk, bootindex, - qemuCaps))) + if (!(optstr = qemuBuildDiskDeviceStr(def, disk, qemuCaps))) return -1; virCommandAddArg(cmd, optstr); @@ -2213,15 +2213,8 @@ qemuBuildDisksCommandLine(virCommand *cmd, for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; - qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - unsigned int bootindex = 0; - - /* The floppy device itself does not support the bootindex property - * so we need to set it up for the controller */ - if (disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) - bootindex = diskPriv->effectiveBootindex; - if (qemuBuildDiskCommandLine(cmd, def, disk, qemuCaps, bootindex) < 0) + if (qemuBuildDiskCommandLine(cmd, def, disk, qemuCaps) < 0) return -1; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 4f1d2bf755..188e63ea1f 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -146,7 +146,6 @@ qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSource *top, char *qemuBuildDiskDeviceStr(const virDomainDef *def, virDomainDiskDef *disk, - unsigned int bootindex, virQEMUCaps *qemuCaps); /* Current, best practice */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a64cddb9e7..7a1b413be0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -734,7 +734,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, goto cleanup; } - if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, 0, priv->qemuCaps))) + if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) goto cleanup; VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
We can skip the formatting of the bootindex for floppies directly at the place where it's being formatted.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 23 ++++++++--------------- src/qemu/qemu_command.h | 1 - src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 9 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There were two negative tests for the keywrapping feature on s390 when the feature flag was missing. For now both shared the error message thus worked fine, but with the upcoming patch to move some disk validation code from the command line formatter to validation code will change the error message in case the disk capabilities are missing. Drop the test cases which don't provide any capability and keep those that have the disk capabilities present as they are sufficient to prove the feature. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 25b0c81f21..ecf63f20fd 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3161,7 +3161,6 @@ mymain(void) DO_TEST_FAILURE("machine-aeskeywrap-on-caps", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-aeskeywrap-on-caps", NONE); DO_TEST("machine-aeskeywrap-on-cap", QEMU_CAPS_AES_KEY_WRAP, @@ -3170,7 +3169,6 @@ mymain(void) DO_TEST_FAILURE("machine-aeskeywrap-on-cap", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-aeskeywrap-on-cap", NONE); DO_TEST("machine-aeskeywrap-off-caps", QEMU_CAPS_AES_KEY_WRAP, QEMU_CAPS_DEA_KEY_WRAP, @@ -3179,7 +3177,6 @@ mymain(void) DO_TEST_FAILURE("machine-aeskeywrap-off-caps", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-aeskeywrap-off-caps", NONE); DO_TEST("machine-aeskeywrap-off-cap", QEMU_CAPS_AES_KEY_WRAP, @@ -3188,7 +3185,6 @@ mymain(void) DO_TEST_FAILURE("machine-aeskeywrap-off-cap", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-aeskeywrap-off-cap", NONE); DO_TEST("machine-deakeywrap-on-caps", QEMU_CAPS_AES_KEY_WRAP, QEMU_CAPS_DEA_KEY_WRAP, @@ -3197,7 +3193,6 @@ mymain(void) DO_TEST_FAILURE("machine-deakeywrap-on-caps", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-deakeywrap-on-caps", NONE); DO_TEST("machine-deakeywrap-on-cap", QEMU_CAPS_DEA_KEY_WRAP, @@ -3206,7 +3201,6 @@ mymain(void) DO_TEST_FAILURE("machine-deakeywrap-on-cap", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-deakeywrap-on-cap", NONE); DO_TEST("machine-deakeywrap-off-caps", QEMU_CAPS_AES_KEY_WRAP, QEMU_CAPS_DEA_KEY_WRAP, @@ -3215,7 +3209,6 @@ mymain(void) DO_TEST_FAILURE("machine-deakeywrap-off-caps", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-deakeywrap-off-caps", NONE); DO_TEST("machine-deakeywrap-off-cap", QEMU_CAPS_DEA_KEY_WRAP, @@ -3224,7 +3217,6 @@ mymain(void) DO_TEST_FAILURE("machine-deakeywrap-off-cap", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390); - DO_TEST_FAILURE("machine-deakeywrap-off-cap", NONE); DO_TEST("machine-keywrap-none-caps", QEMU_CAPS_AES_KEY_WRAP, QEMU_CAPS_DEA_KEY_WRAP, -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
There were two negative tests for the keywrapping feature on s390 when the feature flag was missing. For now both shared the error message thus worked fine, but with the upcoming patch to move some disk validation code from the command line formatter to validation code will change the error message in case the disk capabilities are missing.
Drop the test cases which don't provide any capability and keep those that have the disk capabilities present as they are sufficient to prove the feature.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuxml2argvtest.c | 8 -------- 1 file changed, 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The "machine-loadparm-multiple-disks-nets-s390" case now requires the QEMU_CAPS_CCW feature to pass validation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 50 ---------------------------------------- src/qemu/qemu_validate.c | 49 +++++++++++++++++++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 3 files changed, 50 insertions(+), 51 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 200f9a04b1..9c32fd16b5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1583,50 +1583,6 @@ qemuBuildDriveStr(virDomainDiskDef *disk, } -static bool -qemuCheckIOThreads(const virDomainDef *def, - virDomainDiskDef *disk) -{ - /* Right "type" of disk" */ - switch ((virDomainDiskBus)disk->bus) { - case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads only available for virtio pci and " - "virtio ccw disk")); - return false; - } - break; - - case VIR_DOMAIN_DISK_BUS_IDE: - case VIR_DOMAIN_DISK_BUS_FDC: - case VIR_DOMAIN_DISK_BUS_SCSI: - case VIR_DOMAIN_DISK_BUS_XEN: - case VIR_DOMAIN_DISK_BUS_USB: - case VIR_DOMAIN_DISK_BUS_UML: - case VIR_DOMAIN_DISK_BUS_SATA: - case VIR_DOMAIN_DISK_BUS_SD: - case VIR_DOMAIN_DISK_BUS_NONE: - case VIR_DOMAIN_DISK_BUS_LAST: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOThreads not available for bus %s target %s"), - virDomainDiskBusTypeToString(disk->bus), disk->dst); - return false; - } - - /* Can we find the disk iothread in the iothreadid list? */ - if (!virDomainIOThreadIDFind(def, disk->iothread)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disk iothread '%u' not defined in iothreadid"), - disk->iothread); - return false; - } - - return true; -} - - static int qemuBuildDriveDevCacheStr(virDomainDiskDef *disk, virBuffer *buf, @@ -1668,12 +1624,6 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, g_autofree char *scsiVPDDeviceId = NULL; int controllerModel; - if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst)) - return NULL; - - if (disk->iothread && !qemuCheckIOThreads(def, disk)) - return NULL; - switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index e6ddb43113..9c74092f23 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2404,6 +2404,50 @@ qemuValidateDomainDeviceDefDiskSerial(const char *value) } +static bool +qemuvalidateDomainDeviceDefDiskIOThreads(const virDomainDef *def, + const virDomainDiskDef *disk) +{ + /* Right "type" of disk" */ + switch ((virDomainDiskBus)disk->bus) { + case VIR_DOMAIN_DISK_BUS_VIRTIO: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads only available for virtio pci and " + "virtio ccw disk")); + return false; + } + break; + + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_FDC: + case VIR_DOMAIN_DISK_BUS_SCSI: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_USB: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_SD: + case VIR_DOMAIN_DISK_BUS_NONE: + case VIR_DOMAIN_DISK_BUS_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOThreads not available for bus %s target %s"), + virDomainDiskBusTypeToString(disk->bus), disk->dst); + return false; + } + + /* Can we find the disk iothread in the iothreadid list? */ + if (!virDomainIOThreadIDFind(def, disk->iothread)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Disk iothread '%u' not defined in iothreadid"), + disk->iothread); + return false; + } + + return true; +} + + static int qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, const virDomainDef *def, @@ -2789,6 +2833,11 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk, qemuValidateDomainDeviceDefDiskSerial(disk->serial) < 0) return -1; + if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst)) + return -1; + + if (disk->iothread && !qemuvalidateDomainDeviceDefDiskIOThreads(def, disk)) + return -1; return 0; } diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7af6f90aee..b2bf792743 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -220,7 +220,7 @@ mymain(void) DO_TEST_CAPS_LATEST("genid-auto"); DO_TEST("machine-core-on", NONE); DO_TEST("machine-core-off", NONE); - DO_TEST("machine-loadparm-multiple-disks-nets-s390", NONE); + DO_TEST("machine-loadparm-multiple-disks-nets-s390", QEMU_CAPS_CCW); DO_TEST("default-kvm-host-arch", NONE); DO_TEST("default-qemu-host-arch", NONE); DO_TEST("boot-cdrom", NONE); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
The "machine-loadparm-multiple-disks-nets-s390" case now requires the QEMU_CAPS_CCW feature to pass validation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 50 ---------------------------------------- src/qemu/qemu_validate.c | 49 +++++++++++++++++++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 3 files changed, 50 insertions(+), 51 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Fri, May 21, 2021 at 02:47:12PM +0200, Peter Krempa wrote:
The "machine-loadparm-multiple-disks-nets-s390" case now requires the QEMU_CAPS_CCW feature to pass validation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 50 ---------------------------------------- src/qemu/qemu_validate.c | 49 +++++++++++++++++++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 +- 3 files changed, 50 insertions(+), 51 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 200f9a04b1..9c32fd16b5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1583,50 +1583,6 @@ qemuBuildDriveStr(virDomainDiskDef *disk, }
-static bool -qemuCheckIOThreads(const virDomainDef *def, - virDomainDiskDef *disk) -{ - /* Right "type" of disk" */ - switch ((virDomainDiskBus)disk->bus) { - case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("IOThreads only available for virtio pci and " - "virtio ccw disk")); - return false; - } - break; - - case VIR_DOMAIN_DISK_BUS_IDE: - case VIR_DOMAIN_DISK_BUS_FDC: - case VIR_DOMAIN_DISK_BUS_SCSI: - case VIR_DOMAIN_DISK_BUS_XEN: - case VIR_DOMAIN_DISK_BUS_USB: - case VIR_DOMAIN_DISK_BUS_UML: - case VIR_DOMAIN_DISK_BUS_SATA: - case VIR_DOMAIN_DISK_BUS_SD: - case VIR_DOMAIN_DISK_BUS_NONE: - case VIR_DOMAIN_DISK_BUS_LAST: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("IOThreads not available for bus %s target %s"), - virDomainDiskBusTypeToString(disk->bus), disk->dst); - return false; - } - - /* Can we find the disk iothread in the iothreadid list? */ - if (!virDomainIOThreadIDFind(def, disk->iothread)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Disk iothread '%u' not defined in iothreadid"), - disk->iothread); - return false; - } - - return true; -} - - static int qemuBuildDriveDevCacheStr(virDomainDiskDef *disk, virBuffer *buf, @@ -1668,12 +1624,6 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, g_autofree char *scsiVPDDeviceId = NULL; int controllerModel;
- if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst)) - return NULL; - - if (disk->iothread && !qemuCheckIOThreads(def, disk)) - return NULL; - switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index e6ddb43113..9c74092f23 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2404,6 +2404,50 @@ qemuValidateDomainDeviceDefDiskSerial(const char *value) }
+static bool +qemuvalidateDomainDeviceDefDiskIOThreads(const virDomainDef *def,
s/qemuvalidate/qemuValidate/ Pavel
Pre-extending the disk array size is pointless nowadays since we've switched to memory APIs which don't return failure. Switch all uses of reallocation of the array followed by 'virDomainDiskInsertPreAlloced' with direct virDomainDiskInsert. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_driver.c | 4 +--- src/lxc/lxc_driver.c | 6 +----- src/qemu/qemu_hotplug.c | 3 +-- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d54cd41785..e305f64f9c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3048,8 +3048,6 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, virDomainDeviceDef *dev) goto cleanup; } - VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1); - if (libxlMakeDisk(l_disk, &x_disk) < 0) goto cleanup; @@ -3072,7 +3070,7 @@ libxlDomainAttachDeviceDiskLive(virDomainObj *vm, virDomainDeviceDef *dev) } libxlUpdateDiskDef(l_disk, &x_disk); - virDomainDiskInsertPreAlloced(vm->def, l_disk); + virDomainDiskInsert(vm->def, l_disk); } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 07aeb56b0c..8f2ca19f44 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3401,10 +3401,6 @@ lxcDomainAttachDeviceDiskLive(virLXCDriver *driver, perms) < 0) goto cleanup; - vm->def->disks = g_renew(virDomainDiskDef *, - vm->def->disks, - vm->def->ndisks + 1); - file = g_strdup_printf("/dev/%s", def->dst); if (lxcDomainAttachDeviceMknod(driver, @@ -3423,7 +3419,7 @@ lxcDomainAttachDeviceDiskLive(virLXCDriver *driver, goto cleanup; } - virDomainDiskInsertPreAlloced(vm->def, def); + virDomainDiskInsert(vm->def, def); ret = 0; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7a1b413be0..ee72762d0d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -737,7 +737,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) goto cleanup; - VIR_REALLOC_N(vm->def->disks, vm->def->ndisks + 1); if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; @@ -784,7 +783,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, virDomainAuditDisk(vm, NULL, disk->src, "attach", true); - virDomainDiskInsertPreAlloced(vm->def, disk); + virDomainDiskInsert(vm->def, disk); ret = 0; cleanup: -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Pre-extending the disk array size is pointless nowadays since we've switched to memory APIs which don't return failure.
Switch all uses of reallocation of the array followed by 'virDomainDiskInsertPreAlloced' with direct virDomainDiskInsert.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_driver.c | 4 +--- src/lxc/lxc_driver.c | 6 +----- src/qemu/qemu_hotplug.c | 3 +-- 3 files changed, 3 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Replace the last use of the function by virDomainDiskInsert and remove the unused helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 16 ++-------------- src/conf/domain_conf.h | 2 -- src/libvirt_private.syms | 1 - 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e35c38caa3..4ec6484b78 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15488,13 +15488,6 @@ virDomainDiskByTarget(virDomainDef *def, void virDomainDiskInsert(virDomainDef *def, virDomainDiskDef *disk) -{ - def->disks = g_renew(virDomainDiskDef *, def->disks, def->ndisks + 1); - virDomainDiskInsertPreAlloced(def, disk); -} - -void virDomainDiskInsertPreAlloced(virDomainDef *def, - virDomainDiskDef *disk) { int idx; /* Tentatively plan to insert disk at the end. */ @@ -15521,9 +15514,7 @@ void virDomainDiskInsertPreAlloced(virDomainDef *def, } } - /* VIR_INSERT_ELEMENT_INPLACE will never return an error here. */ - ignore_value(VIR_INSERT_ELEMENT_INPLACE(def->disks, insertAt, - def->ndisks, disk)); + ignore_value(VIR_INSERT_ELEMENT(def->disks, insertAt, def->ndisks, disk)); } @@ -19539,9 +19530,6 @@ virDomainDefParseXML(xmlDocPtr xml, if ((n = virXPathNodeSet("./devices/disk", ctxt, &nodes)) < 0) goto error; - if (n) - def->disks = g_new0(virDomainDiskDef *, n); - for (i = 0; i < n; i++) { virDomainDiskDef *disk = virDomainDiskDefParseXML(xmlopt, nodes[i], @@ -19550,7 +19538,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (!disk) goto error; - virDomainDiskInsertPreAlloced(def, disk); + virDomainDiskInsert(def, disk); } VIR_FREE(nodes); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fab856a5c7..a0855d816e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3548,8 +3548,6 @@ virDomainDiskByTarget(virDomainDef *def, const char *dst); void virDomainDiskInsert(virDomainDef *def, virDomainDiskDef *disk); -void virDomainDiskInsertPreAlloced(virDomainDef *def, - virDomainDiskDef *disk); int virDomainStorageNetworkParseHost(xmlNodePtr hostnode, virStorageNetHostDef *host); int virDomainDiskDefAssignAddress(virDomainXMLOption *xmlopt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6fbdee4124..e6b4ce5bea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -389,7 +389,6 @@ virDomainDiskGetType; virDomainDiskIndexByAddress; virDomainDiskIndexByName; virDomainDiskInsert; -virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskMirrorStateTypeFromString; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Replace the last use of the function by virDomainDiskInsert and remove the unused helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 16 ++-------------- src/conf/domain_conf.h | 2 -- src/libvirt_private.syms | 1 - 3 files changed, 2 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuBlockStorageSourceChainData encapsulates the backend of the disk for startup and hotplug operations. Add the handling for the copy-on-read filter so that the hotplug code doesn't need to have separate cleanup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 12 ++++++++++++ src/qemu/qemu_block.h | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 89f20eb1d2..6627d044cd 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1947,6 +1947,9 @@ qemuBlockStorageSourceChainDataFree(qemuBlockStorageSourceChainData *data) for (i = 0; i < data->nsrcdata; i++) qemuBlockStorageSourceAttachDataFree(data->srcdata[i]); + virJSONValueFree(data->copyOnReadProps); + g_free(data->copyOnReadNodename); + g_free(data->srcdata); g_free(data); } @@ -2054,6 +2057,11 @@ qemuBlockStorageSourceChainAttach(qemuMonitor *mon, return -1; } + if (data->copyOnReadProps) { + if (qemuMonitorBlockdevAdd(mon, &data->copyOnReadProps) < 0) + return -1; + } + return 0; } @@ -2072,6 +2080,10 @@ qemuBlockStorageSourceChainDetach(qemuMonitor *mon, { size_t i; + if (data->copyOnReadAttached) + ignore_value(qemuMonitorBlockdevDel(mon, data->copyOnReadNodename)); + + for (i = 0; i < data->nsrcdata; i++) qemuBlockStorageSourceAttachRollback(mon, data->srcdata[i]); } diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 28ccca97a8..ff7048eb6c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -150,6 +150,10 @@ qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriver *driver, struct _qemuBlockStorageSourceChainData { qemuBlockStorageSourceAttachData **srcdata; size_t nsrcdata; + + virJSONValue *copyOnReadProps; + char *copyOnReadNodename; + bool copyOnReadAttached; }; typedef struct _qemuBlockStorageSourceChainData qemuBlockStorageSourceChainData; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
qemuBlockStorageSourceChainData encapsulates the backend of the disk for startup and hotplug operations. Add the handling for the copy-on-read filter so that the hotplug code doesn't need to have separate cleanup.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 12 ++++++++++++ src/qemu/qemu_block.h | 4 ++++ 2 files changed, 16 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Fill in the required fields in qemuBlockStorageSourceChainData to handle the hotplug so that we can simplify the cleanup code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ee72762d0d..31dc9a43b2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -700,9 +700,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, qemuDomainObjPrivate *priv = vm->privateData; g_autofree char *devstr = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - g_autoptr(virJSONValue) corProps = NULL; - g_autofree char *corAlias = NULL; - bool corAdded = false; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) @@ -718,16 +715,17 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk))) goto cleanup; } else if (blockdev) { + if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src, + priv->qemuCaps))) + goto cleanup; + if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) { - if (!(corProps = qemuBlockStorageGetCopyOnReadProps(disk))) + if (!(data->copyOnReadProps = qemuBlockStorageGetCopyOnReadProps(disk))) goto cleanup; - corAlias = g_strdup(QEMU_DOMAIN_DISK_PRIVATE(disk)->nodeCopyOnRead); + data->copyOnReadNodename = g_strdup(QEMU_DOMAIN_DISK_PRIVATE(disk)->nodeCopyOnRead); } - if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src, - priv->qemuCaps))) - goto cleanup; } else { if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, priv->qemuCaps))) @@ -746,13 +744,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (qemuBlockStorageSourceChainAttach(priv->mon, data) < 0) goto exit_monitor; - if (corProps) { - if (qemuMonitorBlockdevAdd(priv->mon, &corProps) < 0) - goto exit_monitor; - - corAdded = true; - } - if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) goto exit_monitor; @@ -793,8 +784,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, return ret; exit_monitor: - if (corAdded) - ignore_value(qemuMonitorBlockdevDel(priv->mon, corAlias)); qemuBlockStorageSourceChainDetach(priv->mon, data); if (qemuDomainObjExitMonitor(driver, vm) < 0) -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Fill in the required fields in qemuBlockStorageSourceChainData to handle the hotplug so that we can simplify the cleanup code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Unify the handling of the copy-on-read filter by changing the handling to use qemuBlockStorageSourceChainData. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 31dc9a43b2..86b2027be7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4288,7 +4288,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, virDomainDeviceDef dev; size_t i; qemuDomainObjPrivate *priv = vm->privateData; - g_autofree char *corAlias = NULL; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); int ret = -1; @@ -4301,10 +4300,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, if (!(diskBackend = qemuBlockStorageSourceChainDetachPrepareChardev(chardevAlias))) goto cleanup; - } else if (blockdev && - !qemuDiskBusIsSD(disk->bus)) { - corAlias = g_strdup(diskPriv->nodeCopyOnRead); - + } else if (blockdev && !qemuDiskBusIsSD(disk->bus)) { if (diskPriv->blockjob) { /* the block job keeps reference to the disk chain */ diskPriv->blockjob->disk = NULL; @@ -4314,6 +4310,13 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, if (!(diskBackend = qemuBlockStorageSourceChainDetachPrepareBlockdev(disk->src))) goto cleanup; } + + if (diskPriv->nodeCopyOnRead) { + if (!diskBackend) + diskBackend = g_new0(qemuBlockStorageSourceChainData, 1); + diskBackend->copyOnReadNodename = g_strdup(diskPriv->nodeCopyOnRead); + diskBackend->copyOnReadAttached = true; + } } else { char *driveAlias; @@ -4333,9 +4336,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, qemuDomainObjEnterMonitor(driver, vm); - if (corAlias) - ignore_value(qemuMonitorBlockdevDel(priv->mon, corAlias)); - if (diskBackend) qemuBlockStorageSourceChainDetach(priv->mon, diskBackend); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Unify the handling of the copy-on-read filter by changing the handling to use qemuBlockStorageSourceChainData.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the specific device setup and address reservation code into the main hotplug helper as it's just one extra function call. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 86b2027be7..0e3d256fbb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -995,32 +995,16 @@ qemuDomainAttachSCSIDisk(virQEMUDriver *driver, } -static int -qemuDomainAttachUSBMassStorageDevice(virQEMUDriver *driver, - virDomainObj *vm, - virDomainDiskDef *disk) -{ - qemuDomainObjPrivate *priv = vm->privateData; - - if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0) - return -1; - - if (qemuDomainAttachDiskGeneric(driver, vm, disk) < 0) { - virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); - return -1; - } - - return 0; -} - static int qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, virDomainObj *vm, virDomainDeviceDef *dev) { + qemuDomainObjPrivate *priv = vm->privateData; size_t i; virDomainDiskDef *disk = dev->data.disk; + bool releaseUSB = false; int ret = -1; if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || @@ -1060,7 +1044,13 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, _("disk device='lun' is not supported for usb bus")); break; } - ret = qemuDomainAttachUSBMassStorageDevice(driver, vm, disk); + + if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0) + goto cleanup; + + releaseUSB = true; + + ret = qemuDomainAttachDiskGeneric(driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_VIRTIO: @@ -1088,8 +1078,13 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, } cleanup: - if (ret != 0) + if (ret < 0) { ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); + + if (releaseUSB) + virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); + } + return ret; } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Move the specific device setup and address reservation code into the main hotplug helper as it's just one extra function call.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the specific device setup and address reservation code into the main hotplug helper as it's just one extra function call. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0e3d256fbb..da13d84d56 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -798,29 +798,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, } -static int -qemuDomainAttachVirtioDiskDevice(virQEMUDriver *driver, - virDomainObj *vm, - virDomainDiskDef *disk) -{ - virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_DISK, { .disk = disk } }; - bool releaseaddr = false; - int rv; - - if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, disk->dst) < 0) - return -1; - - if ((rv = qemuDomainAttachDiskGeneric(driver, vm, disk)) < 0) { - if (rv == -1 && releaseaddr) - qemuDomainReleaseDeviceAddress(vm, &disk->info); - - return -1; - } - - return 0; -} - - int qemuDomainAttachControllerDevice(virQEMUDriver *driver, virDomainObj *vm, virDomainControllerDef *controller) @@ -1005,6 +982,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, size_t i; virDomainDiskDef *disk = dev->data.disk; bool releaseUSB = false; + bool releaseVirtio = false; int ret = -1; if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || @@ -1054,7 +1032,10 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, break; case VIR_DOMAIN_DISK_BUS_VIRTIO: - ret = qemuDomainAttachVirtioDiskDevice(driver, vm, disk); + if (qemuDomainEnsureVirtioAddress(&releaseVirtio, vm, dev, disk->dst) < 0) + goto cleanup; + + ret = qemuDomainAttachDiskGeneric(driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_SCSI: @@ -1083,6 +1064,9 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (releaseUSB) virDomainUSBAddressRelease(priv->usbaddrs, &disk->info); + + if (releaseVirtio && ret == -1) + qemuDomainReleaseDeviceAddress(vm, &disk->info); } return ret; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Move the specific device setup and address reservation code into the main hotplug helper as it's just one extra function call.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the validation of the SCSI device address and the attachment of the controller into qemuDomainAttachDeviceDiskLiveInternal as there's no specific need for a special helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 69 ++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da13d84d56..8d0ee1c659 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -932,47 +932,6 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriver *driver, } -static int -qemuDomainAttachSCSIDisk(virQEMUDriver *driver, - virDomainObj *vm, - virDomainDiskDef *disk) -{ - size_t i; - - /* We should have an address already, so make sure */ - if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk address type %s"), - virDomainDeviceAddressTypeToString(disk->info.type)); - return -1; - } - - if (virDomainSCSIDriveAddressIsUsed(vm->def, &disk->info.addr.drive)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Domain already contains a disk with that address")); - return -1; - } - - /* Let's make sure the disk has a controller defined and loaded before - * trying to add it. The controller used by the disk must exist before a - * qemu command line string is generated. - * - * Ensure that the given controller and all controllers with a smaller index - * exist; there must not be any missing index in between. - */ - for (i = 0; i <= disk->info.addr.drive.controller; i++) { - if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i)) - return -1; - } - - if (qemuDomainAttachDiskGeneric(driver, vm, disk) < 0) - return -1; - - return 0; -} - - - static int qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, virDomainObj *vm, @@ -1039,7 +998,33 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, break; case VIR_DOMAIN_DISK_BUS_SCSI: - ret = qemuDomainAttachSCSIDisk(driver, vm, disk); + /* We should have an address already, so make sure */ + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk address type %s"), + virDomainDeviceAddressTypeToString(disk->info.type)); + goto cleanup; + } + + if (virDomainSCSIDriveAddressIsUsed(vm->def, &disk->info.addr.drive)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Domain already contains a disk with that address")); + goto cleanup; + } + + /* Let's make sure the disk has a controller defined and loaded before + * trying to add it. The controller used by the disk must exist before a + * qemu command line string is generated. + * + * Ensure that the given controller and all controllers with a smaller index + * exist; there must not be any missing index in between. + */ + for (i = 0; i <= disk->info.addr.drive.controller; i++) { + if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i)) + goto cleanup; + } + + ret = qemuDomainAttachDiskGeneric(driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_IDE: -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Move the validation of the SCSI device address and the attachment of the controller into qemuDomainAttachDeviceDiskLiveInternal as there's no specific need for a special helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 69 ++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 42 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We can call it in one place as all per-device-type subcases use the same code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8d0ee1c659..33c6feea3e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -986,15 +986,11 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, goto cleanup; releaseUSB = true; - - ret = qemuDomainAttachDiskGeneric(driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_VIRTIO: if (qemuDomainEnsureVirtioAddress(&releaseVirtio, vm, dev, disk->dst) < 0) goto cleanup; - - ret = qemuDomainAttachDiskGeneric(driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_SCSI: @@ -1023,8 +1019,6 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i)) goto cleanup; } - - ret = qemuDomainAttachDiskGeneric(driver, vm, disk); break; case VIR_DOMAIN_DISK_BUS_IDE: @@ -1043,6 +1037,8 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, virDomainDiskBusTypeToString(disk->bus)); } + ret = qemuDomainAttachDiskGeneric(driver, vm, disk); + cleanup: if (ret < 0) { ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
We can call it in one place as all per-device-type subcases use the same code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuDomainAttachDeviceDiskLiveInternal already sets up certain pieces of the disk definition so it's better suited to move the setup of the virStorageSource structs, granting access to the storage and allocation of the alias from qemuDomainAttachDiskGeneric which will be just handling the qemu interaction. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 33c6feea3e..fbf4a85c23 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -699,17 +699,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; g_autofree char *devstr = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) - return -1; - - if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto cleanup; - - if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) - goto cleanup; if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) { if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk))) @@ -778,9 +769,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, ret = 0; cleanup: - if (ret < 0) - ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); - qemuDomainSecretDiskDestroy(disk); return ret; exit_monitor: @@ -937,11 +925,13 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, virDomainObj *vm, virDomainDeviceDef *dev) { + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivate *priv = vm->privateData; size_t i; virDomainDiskDef *disk = dev->data.disk; bool releaseUSB = false; bool releaseVirtio = false; + bool releaseSeclabel = false; int ret = -1; if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || @@ -1037,6 +1027,17 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, virDomainDiskBusTypeToString(disk->bus)); } + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0) + goto cleanup; + + releaseSeclabel = true; + + if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) + goto cleanup; + + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + goto cleanup; + ret = qemuDomainAttachDiskGeneric(driver, vm, disk); cleanup: @@ -1048,7 +1049,11 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (releaseVirtio && ret == -1) qemuDomainReleaseDeviceAddress(vm, &disk->info); + + if (releaseSeclabel) + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); } + qemuDomainSecretDiskDestroy(disk); return ret; } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
qemuDomainAttachDeviceDiskLiveInternal already sets up certain pieces of the disk definition so it's better suited to move the setup of the virStorageSource structs, granting access to the storage and allocation of the alias from qemuDomainAttachDiskGeneric which will be just handling the qemu interaction.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Move the auditing entry and insertion into the disk definition from the function which deals with qemu to 'qemuDomainAttachDeviceDiskLiveInternal' which deals with the hotplug related specifics. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index fbf4a85c23..9b7067110e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -763,9 +763,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, goto cleanup; } - virDomainAuditDisk(vm, NULL, disk->src, "attach", true); - - virDomainDiskInsert(vm->def, disk); ret = 0; cleanup: @@ -781,7 +778,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) ret = -2; - virDomainAuditDisk(vm, NULL, disk->src, "attach", false); goto cleanup; } @@ -1040,6 +1036,13 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, ret = qemuDomainAttachDiskGeneric(driver, vm, disk); + virDomainAuditDisk(vm, NULL, disk->src, "attach", ret == 0); + + if (ret < 0) + goto cleanup; + + virDomainDiskInsert(vm->def, disk); + cleanup: if (ret < 0) { ignore_value(qemuRemoveSharedDevice(driver, dev, vm->def->name)); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Move the auditing entry and insertion into the disk definition from the function which deals with qemu to 'qemuDomainAttachDeviceDiskLiveInternal' which deals with the hotplug related specifics.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove two empty lines. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9b7067110e..b1d3ee8e7a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -701,7 +701,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, g_autofree char *devstr = NULL; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) { if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk))) goto cleanup; @@ -726,7 +725,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) goto cleanup; - if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Remove two empty lines.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove the 'ret' variable and 'cleanup' label in favor of directly returning the value since we don't have anything under the 'cleanup:' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b1d3ee8e7a..25e845dc83 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -696,22 +696,21 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, virDomainDiskDef *disk) { g_autoptr(qemuBlockStorageSourceChainData) data = NULL; - int ret = -1; qemuDomainObjPrivate *priv = vm->privateData; g_autofree char *devstr = NULL; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) { if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk))) - goto cleanup; + return -1; } else if (blockdev) { if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src, priv->qemuCaps))) - goto cleanup; + return -1; if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) { if (!(data->copyOnReadProps = qemuBlockStorageGetCopyOnReadProps(disk))) - goto cleanup; + return -1; data->copyOnReadNodename = g_strdup(QEMU_DOMAIN_DISK_PRIVATE(disk)->nodeCopyOnRead); } @@ -719,14 +718,14 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, } else { if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, priv->qemuCaps))) - goto cleanup; + return -1; } if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) - goto cleanup; + return -1; if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) - goto cleanup; + return -1; qemuDomainObjEnterMonitor(driver, vm); @@ -756,27 +755,22 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, VIR_WARN("failed to set blkdeviotune for '%s' of '%s'", disk->dst, vm->def->name); } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - ret = -2; - goto cleanup; - } - - ret = 0; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -2; - cleanup: - return ret; + return 0; exit_monitor: qemuBlockStorageSourceChainDetach(priv->mon, data); if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -2; + return -2; if (virStorageSourceChainHasManagedPR(disk->src) && qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) - ret = -2; + return -2; - goto cleanup; + return -1; } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Remove the 'ret' variable and 'cleanup' label in favor of directly returning the value since we don't have anything under the 'cleanup:' label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Similarly to previous refactors we want to move all hotplug related setup which isn't strictly relevant to attaching the disk into qemuDomainAttachDeviceDiskLiveInternal. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 25e845dc83..f8c741c683 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -724,9 +724,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) return -1; - if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) - return -1; - qemuDomainObjEnterMonitor(driver, vm); if (qemuBlockStorageSourceChainAttach(priv->mon, data) < 0) @@ -766,10 +763,6 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) return -2; - if (virStorageSourceChainHasManagedPR(disk->src) && - qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) - return -2; - return -1; } @@ -1026,6 +1019,9 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto cleanup; + if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + ret = qemuDomainAttachDiskGeneric(driver, vm, disk); virDomainAuditDisk(vm, NULL, disk->src, "attach", ret == 0); @@ -1047,6 +1043,9 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (releaseSeclabel) ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, disk->src)); + + if (virStorageSourceChainHasManagedPR(disk->src)) + ignore_value(qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE)); } qemuDomainSecretDiskDestroy(disk); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Similarly to previous refactors we want to move all hotplug related setup which isn't strictly relevant to attaching the disk into qemuDomainAttachDeviceDiskLiveInternal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Modify the rollback section to use it's own monitor context so that we can later split up the hotplug into multiple steps and move the detachment of the extension device into the rollback section rather than doing it inline. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f8c741c683..3772c5b4b8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -699,6 +699,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, qemuDomainObjPrivate *priv = vm->privateData; g_autofree char *devstr = NULL; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); + bool extensionDeviceAttached = false; + int rc; if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) { if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk))) @@ -726,16 +728,14 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, qemuDomainObjEnterMonitor(driver, vm); - if (qemuBlockStorageSourceChainAttach(priv->mon, data) < 0) - goto exit_monitor; + rc = qemuBlockStorageSourceChainAttach(priv->mon, data); - if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0) - goto exit_monitor; + if (rc == 0 && + (rc = qemuDomainAttachExtensionDevice(priv->mon, &disk->info)) == 0) + extensionDeviceAttached = true; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) { - ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); - goto exit_monitor; - } + if (rc == 0) + rc = qemuMonitorAddDevice(priv->mon, devstr); /* Setup throttling of disk via block_set_io_throttle QMP command. This * is a hack until the 'throttle' blockdev driver will support modification @@ -743,7 +743,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, * As there isn't anything sane to do if this fails, let's just return * success. */ - if (blockdev && + if (blockdev && rc == 0 && qemuDiskConfigBlkdeviotuneEnabled(disk)) { qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); if (qemuMonitorSetBlockIoThrottle(priv->mon, NULL, diskPriv->qomName, @@ -755,9 +755,17 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) return -2; + if (rc < 0) + goto rollback; + return 0; - exit_monitor: + rollback: + qemuDomainObjEnterMonitor(driver, vm); + + if (extensionDeviceAttached) + ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); + qemuBlockStorageSourceChainDetach(priv->mon, data); if (qemuDomainObjExitMonitor(driver, vm) < 0) -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Modify the rollback section to use it's own monitor context so that we
*its
can later split up the hotplug into multiple steps and move the detachment of the extension device into the rollback section rather than doing it inline.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Split up the monitor contexts to attach the backend of the disk and the frontend device in preparation for hotplugging transient disks where we'll need to add the code for adding the transient overlay between these two steps. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3772c5b4b8..e13a739ade 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -723,15 +723,22 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, return -1; } - if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) - return -1; - qemuDomainObjEnterMonitor(driver, vm); rc = qemuBlockStorageSourceChainAttach(priv->mon, data); - if (rc == 0 && - (rc = qemuDomainAttachExtensionDevice(priv->mon, &disk->info)) == 0) + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -2; + + if (rc < 0) + goto rollback; + + if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) + goto rollback; + + qemuDomainObjEnterMonitor(driver, vm); + + if ((rc = qemuDomainAttachExtensionDevice(priv->mon, &disk->info)) == 0) extensionDeviceAttached = true; if (rc == 0) -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Split up the monitor contexts to attach the backend of the disk and the frontend device in preparation for hotplugging transient disks where we'll need to add the code for adding the transient overlay between these two steps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In preparation for hotplug of <transient> disks we'll need to track whether the overlay file was created individually per-disk. Add 'transientOverlayCreated' to 'struct _qemuDomainDiskPrivate' and remove 'inhibitDiskTransientDelete' from 'qemuDomainObjPrivate' and adjust the code for the change. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 7 +++---- src/qemu/qemu_process.c | 33 +++++++++++++++++++-------------- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 10641846b3..c5f994c4a5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1880,8 +1880,6 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivate *priv) g_slist_free_full(g_steal_pointer(&priv->dbusVMStateIds), g_free); priv->dbusVMState = false; - - priv->inhibitDiskTransientDelete = false; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 4fc1969d9e..9cebf9e426 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -258,10 +258,6 @@ struct _qemuDomainObjPrivate { GSList *dbusVMStateIds; /* true if -object dbus-vmstate was added */ bool dbusVMState; - - /* prevent deletion of <transient> disk overlay files between startup and - * successful setup of the overlays */ - bool inhibitDiskTransientDelete; }; #define QEMU_DOMAIN_PRIVATE(vm) \ @@ -292,6 +288,9 @@ struct _qemuDomainDiskPrivate { unsigned int effectiveBootindex; /* boot index of the disk based on one of the two ways we use to select a boot device */ + + bool transientOverlayCreated; /* the overlay image of a transient disk was + created and the definition was updated */ }; #define QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7fe051505b..9302180403 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5709,9 +5709,6 @@ qemuProcessInit(virQEMUDriver *driver, if (virDomainObjSetDefTransient(driver->xmlopt, vm, priv->qemuCaps) < 0) goto cleanup; - /* don't clean up files for <transient> disks until we set them up */ - priv->inhibitDiskTransientDelete = true; - if (flags & VIR_QEMU_PROCESS_START_PRETEND) { if (qemuDomainSetPrivatePaths(driver, vm) < 0) { virDomainObjRemoveTransientDef(vm); @@ -7007,7 +7004,6 @@ static int qemuProcessSetupDisksTransientSnapshot(virDomainObj *vm, qemuDomainAsyncJob asyncJob) { - qemuDomainObjPrivate *priv = vm->privateData; g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; size_t i; @@ -7039,8 +7035,14 @@ qemuProcessSetupDisksTransientSnapshot(virDomainObj *vm, if (qemuSnapshotDiskCreate(snapctxt) < 0) return -1; - /* the overlays are established, so they can be deleted on shutdown */ - priv->inhibitDiskTransientDelete = false; + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *domdisk = vm->def->disks[i]; + + if (!domdisk->transient) + continue; + + QEMU_DOMAIN_DISK_PRIVATE(domdisk)->transientOverlayCreated = true; + } return 0; } @@ -8077,7 +8079,7 @@ void qemuProcessStop(virQEMUDriver *driver, /* for now transient disks are forbidden with migration so they * can be handled here */ if (disk->transient && - !priv->inhibitDiskTransientDelete) { + QEMU_DOMAIN_DISK_PRIVATE(disk)->transientOverlayCreated) { VIR_DEBUG("Removing transient overlay '%s' of disk '%s'", disk->src->path, disk->dst); if (qemuDomainStorageFileInit(driver, vm, disk->src, NULL) >= 0) { @@ -8505,10 +8507,6 @@ qemuProcessReconnect(void *opaque) if (oldjob.asyncJob == QEMU_ASYNC_JOB_BACKUP && priv->backup) priv->backup->apiFlags = oldjob.apiFlags; - /* expect that libvirt might have crashed during VM start, so prevent - * cleanup of transient disks */ - priv->inhibitDiskTransientDelete = true; - if (qemuDomainObjBeginJob(driver, obj, QEMU_JOB_MODIFY) < 0) goto error; jobStarted = true; @@ -8611,9 +8609,6 @@ qemuProcessReconnect(void *opaque) goto error; } - /* vm startup complete, we can remove transient disks if required */ - priv->inhibitDiskTransientDelete = false; - /* In case the domain shutdown while we were not running, * we need to finish the shutdown process. And we need to do it after * we have virQEMUCaps filled in. @@ -8662,6 +8657,16 @@ qemuProcessReconnect(void *opaque) if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error; + /* At this point we've already checked that the startup of the VM was + * completed successfully before, thus that also implies that all transient + * disk overlays were created. */ + for (i = 0; i < obj->def->ndisks; i++) { + virDomainDiskDef *disk = obj->def->disks[i]; + + if (disk->transient) + QEMU_DOMAIN_DISK_PRIVATE(disk)->transientOverlayCreated = true; + } + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV) && qemuBlockNodeNamesDetect(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
In preparation for hotplug of <transient> disks we'll need to track whether the overlay file was created individually per-disk.
Add 'transientOverlayCreated' to 'struct _qemuDomainDiskPrivate' and remove 'inhibitDiskTransientDelete' from 'qemuDomainObjPrivate' and adjust the code for the change.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 7 +++---- src/qemu/qemu_process.c | 33 +++++++++++++++++++-------------- 3 files changed, 22 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add code which creates the transient overlay after hotplugging the disk backend before attaching the disk frontend. The state of the topmost image is modified to be already read-only to prevent the need to open the image in read-write mode. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 52 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e13a739ade..59832ea2b3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -34,6 +34,7 @@ #include "qemu_process.h" #include "qemu_security.h" #include "qemu_block.h" +#include "qemu_snapshot.h" #include "domain_audit.h" #include "netdev_bandwidth_conf.h" #include "domain_nwfilter.h" @@ -685,6 +686,25 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, } +static qemuSnapshotDiskContext * +qemuDomainAttachDiskGenericTransient(virDomainObj *vm, + virDomainDiskDef *disk, + GHashTable *blockNamedNodeData) +{ + g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; + g_autoptr(virDomainSnapshotDiskDef) snapdiskdef = NULL; + + snapdiskdef = qemuSnapshotGetTransientDiskDef(disk, vm->def->name); + snapctxt = qemuSnapshotDiskContextNew(1, vm, QEMU_ASYNC_JOB_NONE); + + if (qemuSnapshotDiskPrepareOne(snapctxt, disk, snapdiskdef, + blockNamedNodeData, false, false) < 0) + return NULL; + + return g_steal_pointer(&snapctxt); +} + + /** * qemuDomainAttachDiskGeneric: * @@ -701,6 +721,11 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); bool extensionDeviceAttached = false; int rc; + g_autoptr(qemuSnapshotDiskContext) transientDiskSnapshotCtxt = NULL; + bool origReadonly = disk->src->readonly; + + if (disk->transient) + disk->src->readonly = true; if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) { if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk))) @@ -723,6 +748,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, return -1; } + disk->src->readonly = origReadonly; + qemuDomainObjEnterMonitor(driver, vm); rc = qemuBlockStorageSourceChainAttach(priv->mon, data); @@ -733,6 +760,25 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (rc < 0) goto rollback; + if (disk->transient) { + g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL; + g_autoptr(GHashTable) blockNamedNodeData = NULL; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) + goto rollback; + + if (!(transientDiskSnapshotCtxt = qemuDomainAttachDiskGenericTransient(vm, disk, blockNamedNodeData))) + goto rollback; + + + if (qemuSnapshotDiskCreate(transientDiskSnapshotCtxt) < 0) + goto rollback; + + QEMU_DOMAIN_DISK_PRIVATE(disk)->transientOverlayCreated = true; + backend = qemuBlockStorageSourceDetachPrepare(disk->src, NULL); + ignore_value(VIR_INSERT_ELEMENT(data->srcdata, 0, data->nsrcdata, backend)); + } + if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) goto rollback; @@ -937,12 +983,6 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, return -1; } - if (disk->transient) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("transient disk hotplug isn't supported")); - return -1; - } - if (virDomainDiskTranslateSourcePool(disk) < 0) goto cleanup; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Add code which creates the transient overlay after hotplugging the disk backend before attaching the disk frontend.
The state of the topmost image is modified to be already read-only to prevent the need to open the image in read-write mode.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 52 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The qemuDomainAttachDiskGeneric will also be used on startup for transient disks which share the overlay. The VM startup code passes the asyncJob around so we need to pass it into qemuDomainAttachDiskGeneric. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 59832ea2b3..03649634c5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -689,13 +689,14 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, static qemuSnapshotDiskContext * qemuDomainAttachDiskGenericTransient(virDomainObj *vm, virDomainDiskDef *disk, - GHashTable *blockNamedNodeData) + GHashTable *blockNamedNodeData, + qemuDomainAsyncJob asyncJob) { g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; g_autoptr(virDomainSnapshotDiskDef) snapdiskdef = NULL; snapdiskdef = qemuSnapshotGetTransientDiskDef(disk, vm->def->name); - snapctxt = qemuSnapshotDiskContextNew(1, vm, QEMU_ASYNC_JOB_NONE); + snapctxt = qemuSnapshotDiskContextNew(1, vm, asyncJob); if (qemuSnapshotDiskPrepareOne(snapctxt, disk, snapdiskdef, blockNamedNodeData, false, false) < 0) @@ -713,7 +714,8 @@ qemuDomainAttachDiskGenericTransient(virDomainObj *vm, static int qemuDomainAttachDiskGeneric(virQEMUDriver *driver, virDomainObj *vm, - virDomainDiskDef *disk) + virDomainDiskDef *disk, + qemuDomainAsyncJob asyncJob) { g_autoptr(qemuBlockStorageSourceChainData) data = NULL; qemuDomainObjPrivate *priv = vm->privateData; @@ -750,7 +752,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, disk->src->readonly = origReadonly; - qemuDomainObjEnterMonitor(driver, vm); + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; rc = qemuBlockStorageSourceChainAttach(priv->mon, data); @@ -764,10 +767,10 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; - if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) goto rollback; - if (!(transientDiskSnapshotCtxt = qemuDomainAttachDiskGenericTransient(vm, disk, blockNamedNodeData))) + if (!(transientDiskSnapshotCtxt = qemuDomainAttachDiskGenericTransient(vm, disk, blockNamedNodeData, asyncJob))) goto rollback; @@ -782,7 +785,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, if (!(devstr = qemuBuildDiskDeviceStr(vm->def, disk, priv->qemuCaps))) goto rollback; - qemuDomainObjEnterMonitor(driver, vm); + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto rollback; if ((rc = qemuDomainAttachExtensionDevice(priv->mon, &disk->info)) == 0) extensionDeviceAttached = true; @@ -814,7 +818,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriver *driver, return 0; rollback: - qemuDomainObjEnterMonitor(driver, vm); + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; if (extensionDeviceAttached) ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info)); @@ -1077,7 +1082,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, if (qemuHotplugAttachManagedPR(driver, vm, disk->src, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; - ret = qemuDomainAttachDiskGeneric(driver, vm, disk); + ret = qemuDomainAttachDiskGeneric(driver, vm, disk, QEMU_ASYNC_JOB_NONE); virDomainAuditDisk(vm, NULL, disk->src, "attach", ret == 0); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
The qemuDomainAttachDiskGeneric will also be used on startup for transient disks which share the overlay. The VM startup code passes the asyncJob around so we need to pass it into qemuDomainAttachDiskGeneric.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_hotplug.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 03649634c5..b91cab08ac 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -711,7 +711,7 @@ qemuDomainAttachDiskGenericTransient(virDomainObj *vm, * * Attaches disk to a VM. This function aggregates common code for all bus types. * In cases when the VM crashed while adding the disk, -2 is returned. */ -static int +int qemuDomainAttachDiskGeneric(virQEMUDriver *driver, virDomainObj *vm, virDomainDiskDef *disk, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index df8f76f8d6..b5f7afb076 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -58,6 +58,12 @@ int qemuDomainAttachControllerDevice(virQEMUDriver *driver, int qemuDomainAttachDeviceDiskLive(virQEMUDriver *driver, virDomainObj *vm, virDomainDeviceDef *dev); + +int qemuDomainAttachDiskGeneric(virQEMUDriver *driver, + virDomainObj *vm, + virDomainDiskDef *disk, + qemuDomainAsyncJob asyncJob); + int qemuDomainAttachNetDevice(virQEMUDriver *driver, virDomainObj *vm, virDomainNetDef *net); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_hotplug.h | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In case the user wants to share the disk image between multiple VMs the qemu driver needs to hotplug such disks to instantiate the backends. Since that doesn't work for all disk configs add a switch to force this behaviour. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 6 ++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 17 ++++++++++++++--- src/conf/domain_conf.h | 1 + 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index fa5c14febc..61fa520965 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3135,6 +3135,12 @@ paravirtualized driver is specified via the ``disk`` element. disk transient prevents the domain from participating in migration, snapshots, or blockjobs. Only supported in vmx hypervisor (:since:`Since 0.9.5`) and ``qemu`` hypervisor (:since:`Since 6.9.0`). + + In cases where the source image of the ``<transient/>`` disk is supposed to + be shared between multiple concurrently running VMs the optional + ``shareBacking`` attribute should be set to ``yes``. Note that hypervisor + drivers may need to hotplug such disk and thus it works only with + configurations supporting hotplug. :since:`Since 7.4.0` ``serial`` If present, this specify serial number of virtual hard drive. For example, it may look like ``<serial>WD-WMAP9A966149</serial>``. Not supported for diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index a2e5c50c1d..e476eca250 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1429,6 +1429,11 @@ </optional> <optional> <element name="transient"> + <optional> + <attribute name="shareBacking"> + <ref name='virYesNo'/> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4ec6484b78..749c07b2c1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9038,6 +9038,7 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt, xmlNodePtr blockioNode; xmlNodePtr driverNode; xmlNodePtr mirrorNode; + xmlNodePtr transientNode; g_autoptr(virStorageSource) src = NULL; if (!(src = virDomainDiskDefParseSourceXML(xmlopt, node, ctxt, flags))) @@ -9146,9 +9147,15 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt, } } - if (virXPathNode("./transient", ctxt)) + if ((transientNode = virXPathNode("./transient", ctxt))) { def->transient = true; + if (virXMLPropTristateBool(transientNode, "shareBacking", + VIR_XML_PROP_NONE, + &def->transientShareBacking) < 0) + return NULL; + } + if (virDomainDiskDefIotuneParse(def, ctxt) < 0) return NULL; @@ -23540,8 +23547,12 @@ virDomainDiskDefFormat(virBuffer *buf, virBufferAddLit(buf, "<readonly/>\n"); if (def->src->shared) virBufferAddLit(buf, "<shareable/>\n"); - if (def->transient) - virBufferAddLit(buf, "<transient/>\n"); + if (def->transient) { + virBufferAddLit(buf, "<transient"); + if (def->transientShareBacking == VIR_TRISTATE_BOOL_YES) + virBufferAddLit(buf, " shareBacking='yes'"); + virBufferAddLit(buf, "/>\n"); + } virBufferEscapeString(buf, "<serial>%s</serial>\n", def->serial); virBufferEscapeString(buf, "<wwn>%s</wwn>\n", def->wwn); virBufferEscapeString(buf, "<vendor>%s</vendor>\n", def->vendor); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a0855d816e..34420f0a10 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -575,6 +575,7 @@ struct _virDomainDiskDef { unsigned int snapshot; /* virDomainSnapshotLocation, snapshot_conf.h */ virDomainStartupPolicy startupPolicy; bool transient; + virTristateBool transientShareBacking; virDomainDeviceInfo info; virTristateBool rawio; virDomainDeviceSGIO sgio; -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
In case the user wants to share the disk image between multiple VMs the qemu driver needs to hotplug such disks to instantiate the backends. Since that doesn't work for all disk configs add a switch to force this behaviour.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 6 ++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 17 ++++++++++++++--- src/conf/domain_conf.h | 1 + 4 files changed, 26 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Implement this behaviour by skipping the disks on traditional commandline and hotplug them before resuming CPUs. That allows to use the support for hotplugging of transient disks which inherently allows sharing of the backing image as we open it read-only. This commit implements the validation code to allow it only with buses supporting hotplug and the hotplug code while starting up the VM. When we have such disk we need to issue a system-reset so that firmware tables are regenerated to allow booting from such device. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 6 ++++++ src/qemu/qemu_process.c | 45 +++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_validate.c | 24 +++++++++++++++++++++ 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9c32fd16b5..697324eea5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2164,6 +2164,12 @@ qemuBuildDisksCommandLine(virCommand *cmd, for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; + /* transient disks with shared backing image will be hotplugged after + * the VM is started */ + if (disk->transient && + disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) + continue; + if (qemuBuildDiskCommandLine(cmd, def, disk, qemuCaps) < 0) return -1; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9302180403..c0b4f3530f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7017,7 +7017,8 @@ qemuProcessSetupDisksTransientSnapshot(virDomainObj *vm, virDomainDiskDef *domdisk = vm->def->disks[i]; g_autoptr(virDomainSnapshotDiskDef) snapdisk = NULL; - if (!domdisk->transient) + if (!domdisk->transient || + domdisk->transientShareBacking == VIR_TRISTATE_BOOL_YES) continue; /* validation code makes sure that we do this only for local disks @@ -7048,6 +7049,45 @@ qemuProcessSetupDisksTransientSnapshot(virDomainObj *vm, } +static int +qemuProcessSetupDisksTransientHotplug(virDomainObj *vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivate *priv = vm->privateData; + bool hasHotpluggedDisk = false; + size_t i; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *domdisk = vm->def->disks[i]; + + if (!domdisk->transient || + domdisk->transientShareBacking != VIR_TRISTATE_BOOL_YES) + continue; + + if (qemuDomainAttachDiskGeneric(priv->driver, vm, domdisk, asyncJob) < 0) + return -1; + + hasHotpluggedDisk = true; + } + + /* in order to allow booting from such disks we need to issue a system-reset + * so that the firmware tables recording bootable devices are regerated */ + if (hasHotpluggedDisk) { + int rc; + + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return -1; + + rc = qemuMonitorSystemReset(priv->mon); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || rc < 0) + return -1; + } + + return 0; +} + + static int qemuProcessSetupDisksTransient(virDomainObj *vm, qemuDomainAsyncJob asyncJob) @@ -7060,6 +7100,9 @@ qemuProcessSetupDisksTransient(virDomainObj *vm, if (qemuProcessSetupDisksTransientSnapshot(vm, asyncJob) < 0) return -1; + if (qemuProcessSetupDisksTransientHotplug(vm, asyncJob) < 0) + return -1; + return 0; } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 9c74092f23..6d6bca859b 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -2980,6 +2980,30 @@ qemuValidateDomainDeviceDefDiskTransient(const virDomainDiskDef *disk, return -1; } + if (disk->transientShareBacking == VIR_TRISTATE_BOOL_YES) { + /* sharing the backing file requires hotplug of the disk in the qemu driver */ + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_USB: + case VIR_DOMAIN_DISK_BUS_VIRTIO: + case VIR_DOMAIN_DISK_BUS_SCSI: + break; + + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_FDC: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_SD: + case VIR_DOMAIN_DISK_BUS_NONE: + case VIR_DOMAIN_DISK_BUS_LAST: + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk bus '%s' doesn't support transiend disk backing image sharing"), + virDomainDiskBusTypeToString(disk->bus)); + return -1; + } + } + return 0; } -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Implement this behaviour by skipping the disks on traditional commandline and hotplug them before resuming CPUs. That allows to use the support for hotplugging of transient disks which inherently allows sharing of the backing image as we open it read-only.
This commit implements the validation code to allow it only with buses supporting hotplug and the hotplug code while starting up the VM.
When we have such disk we need to issue a system-reset so that firmware tables are regenerated to allow booting from such device.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 6 ++++++ src/qemu/qemu_process.c | 45 +++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_validate.c | 24 +++++++++++++++++++++ 3 files changed, 74 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-transient.x86_64-4.1.0.err | 2 +- .../disk-transient.x86_64-latest.args | 4 +- tests/qemuxml2argvdata/disk-transient.xml | 6 +++ .../disk-transient.x86_64-latest.xml | 48 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/disk-transient.x86_64-latest.xml diff --git a/tests/qemuxml2argvdata/disk-transient.x86_64-4.1.0.err b/tests/qemuxml2argvdata/disk-transient.x86_64-4.1.0.err index eb5af4762d..493da051e3 100644 --- a/tests/qemuxml2argvdata/disk-transient.x86_64-4.1.0.err +++ b/tests/qemuxml2argvdata/disk-transient.x86_64-4.1.0.err @@ -1 +1 @@ -unsupported configuration: transient disk not supported by this QEMU binary (hda) +unsupported configuration: transient disk not supported by this QEMU binary (vda) diff --git a/tests/qemuxml2argvdata/disk-transient.x86_64-latest.args b/tests/qemuxml2argvdata/disk-transient.x86_64-latest.args index 41f6400891..18f16c3489 100644 --- a/tests/qemuxml2argvdata/disk-transient.x86_64-latest.args +++ b/tests/qemuxml2argvdata/disk-transient.x86_64-latest.args @@ -29,8 +29,8 @@ XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -blockdev '{"driver":"file","filename":"/tmp/QEMUGuest1.img","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}' \ -blockdev '{"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}' \ --device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1,write-cache=on \ +-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,write-cache=on \ -audiodev id=audio1,driver=none \ --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/disk-transient.xml b/tests/qemuxml2argvdata/disk-transient.xml index cffb325336..49613a88b2 100644 --- a/tests/qemuxml2argvdata/disk-transient.xml +++ b/tests/qemuxml2argvdata/disk-transient.xml @@ -14,6 +14,12 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-i386</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/tmp/QEMUGuest2.img'/> + <target dev='vda' bus='virtio'/> + <transient shareBacking='yes'/> + </disk> <disk type='file' device='disk'> <driver name='qemu' type='qcow2' cache='none'/> <source file='/tmp/QEMUGuest1.img'/> diff --git a/tests/qemuxml2xmloutdata/disk-transient.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-transient.x86_64-latest.xml new file mode 100644 index 0000000000..988706002d --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-transient.x86_64-latest.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i386</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/tmp/QEMUGuest2.img'/> + <target dev='vda' bus='virtio'/> + <transient shareBacking='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/tmp/QEMUGuest1.img'/> + <target dev='hda' bus='ide'/> + <transient/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0' model='piix3-uhci'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b2bf792743..da85e860a4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -307,6 +307,7 @@ mymain(void) DO_TEST("disk-boot-disk", NONE); DO_TEST("disk-boot-cdrom", NONE); DO_TEST("disk-error-policy", NONE); + DO_TEST_CAPS_LATEST("disk-transient"); DO_TEST("disk-fmt-qcow", NONE); DO_TEST_CAPS_VER("disk-cache", "2.12.0"); DO_TEST_CAPS_LATEST("disk-cache"); -- 2.31.1
On a Friday in 2021, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- .../disk-transient.x86_64-4.1.0.err | 2 +- .../disk-transient.x86_64-latest.args | 4 +- tests/qemuxml2argvdata/disk-transient.xml | 6 +++ .../disk-transient.x86_64-latest.xml | 48 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 58 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/disk-transient.x86_64-latest.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Fri, May 21, 2021 at 02:47:00PM +0200, Peter Krempa wrote:
This series refactors the qemu disk hotplug code so that it can be easily extended for hotplug of transient disks and then also used at startup to plug in disks where the users want to share the backing image.
Masayoshi Mizuma (1): qemu_snapshot: Add the guest name to the transient disk path
Peter Krempa (34): qemu: snapshot: Extract setup of snapshot disk definition for transient disks qemu: process: Setup transient disks only when starting a fresh VM qemuSnapshotDiskPrepareOne: Pass in qemuSnapshotDiskContext qemuSnapshotDiskContext: Store virQEMUDriverConfig in the struct qemuSnapshotDiskPrepareOne: Use data from qemuSnapshotDiskContext qemuSnapshotDiskCreate: Use 'cfg' from the qemuSnapshotDiskContext qemu: snapshot: move transient snapshot code to qemu_process qemu: Move 'bootindex' handling for disks out of command line formatter qemu: Move bootindex usage logic into qemuBuildDiskDeviceStr qemuxml2argvtest: Remove pointless tests for keywrapping on s390 qemu: Move iothread and s390 address validation for disk devices into the validator Replace virDomainDiskInsertPreAlloced by virDomainDiskInsert conf: remove virDomainDiskInsertPreAlloced qemuBlockStorageSourceChainData: Add handling of 'copy-on-read' filter layer qemuDomainAttachDiskGeneric: Move 'copy-on-read' handling to qemuBlockStorageSourceChainData qemuDomainRemoveDiskDevice: Move 'copy-on-read' handling to qemuBlockStorageSourceChainData qemuDomainAttachDeviceDiskLiveInternal: Absorb qemuDomainAttachUSBMassStorageDevice qemuDomainAttachDeviceDiskLiveInternal: Absorb qemuDomainAttachVirtioDiskDevice qemuDomainAttachDeviceDiskLiveInternal: Absorb qemuDomainAttachSCSIDisk qemuDomainAttachDeviceDiskLiveInternal: Simplify call to qemuDomainAttachDiskGeneric qemuDomainAttachDiskGeneric: Move setup of disk into qemuDomainAttachDeviceDiskLiveInternal qemu: hotplug: Move post-insertion steps of disk hotplug to qemuDomainAttachDeviceDiskLiveInternal qemuDomainAttachDiskGeneric: Fix whitespace qemuDomainAttachDiskGeneric: Refactor cleanup qemuDomainAttachDiskGeneric: Move PR helper attach into qemuDomainAttachDeviceDiskLiveInternal qemuDomainAttachDiskGeneric: Refactor rollback handling qemuDomainAttachDiskGeneric: Split up frontend and backend attachment qemu: Track creation of <transient> disk overlay individually qemuDomainAttachDiskGeneric: Implement hotplug of <transient> disk qemuDomainAttachDiskGeneric: Pass the qemu async job type qemuDomainAttachDiskGeneric: Export conf: Introduce 'shareBacking' for <transient> disks qemu: Allow <transient> disks with images shared accross VMs tests: Add qemuxml2argv and qemuxml2xml test for <transient shareBacking='yes'>
With the issues fixed Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (3)
-
Ján Tomko -
Pavel Hrdina -
Peter Krempa