[PATCH 00/10] qemu: Fix pre-creation of non-shared storage on migration
The last patch fixes a bug where we'd fail to detect capacity of disks on migration source due to changes in disk topology related to blockdev. The rest of the series makes the function at least somewhat sane. Peter Krempa (10): util: hash: Use g_new0 for allocating hash internals conf: domain: Remove checking of return value of virHashCreateFull Remove checking of return value of virHashNew qemuMigrationCookieAddNBD: Exit early if there are no disks qemuMigrationCookieNBD: Extract embedded struct qemuMigrationCookieAddNBD: Use glib memory allocators qemuMigrationCookieAddNBD: Move monitor call out of the loop qemuMigrationCookieAddNBD: Use virHashNew and automatic freeing of virHashTablePtr qemuMigrationCookieAddNBD: Remove 'ret' variable and 'cleanup' label qemuMigrationCookieAddNBD: Fix filling of 'capacity' when blockdev is used src/conf/backup_conf.c | 6 +--- src/conf/domain_addr.c | 32 +++++++----------- src/qemu/qemu_migration_cookie.c | 58 +++++++++++++++----------------- src/qemu/qemu_migration_cookie.h | 10 +++--- src/qemu/qemu_monitor_json.c | 3 +- src/util/virhash.c | 14 +++----- 6 files changed, 54 insertions(+), 69 deletions(-) -- 2.24.1
Use the glib helpers and remove the mention of returning NULL on failure of virHashNew, virHashCreate and virHashCreateFull. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhash.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/util/virhash.c b/src/util/virhash.c index edf11e8b7a..d5c5e017a1 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -138,7 +138,7 @@ virHashComputeKey(const virHashTable *table, const void *name) * * Create a new virHashTablePtr. * - * Returns the newly created object, or NULL if an error occurred. + * Returns the newly created object. */ virHashTablePtr virHashCreateFull(ssize_t size, virHashDataFree dataFree, @@ -153,8 +153,7 @@ virHashTablePtr virHashCreateFull(ssize_t size, if (size <= 0) size = 256; - if (VIR_ALLOC(table) < 0) - return NULL; + table = g_new0(virHashTable, 1); table->seed = virRandomBits(32); table->size = size; @@ -166,10 +165,7 @@ virHashTablePtr virHashCreateFull(ssize_t size, table->keyPrint = keyPrint; table->keyFree = keyFree; - if (VIR_ALLOC_N(table->table, size) < 0) { - VIR_FREE(table); - return NULL; - } + table->table = g_new0(virHashEntryPtr, table->size); return table; } @@ -181,7 +177,7 @@ virHashTablePtr virHashCreateFull(ssize_t size, * * Create a new virHashTablePtr. * - * Returns the newly created object, or NULL if an error occurred. + * Returns the newly created object. */ virHashTablePtr virHashNew(virHashDataFree dataFree) @@ -203,7 +199,7 @@ virHashNew(virHashDataFree dataFree) * * Create a new virHashTablePtr. * - * Returns the newly created object, or NULL if an error occurred. + * Returns the newly created object. */ virHashTablePtr virHashCreate(ssize_t size, virHashDataFree dataFree) { -- 2.24.1
This module has last two direct checks whether the value returned by virHashCreateFull is NULL. Remove them so that static analyzers don't get the false idea that checking the value is necessary. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_addr.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 607ba56efd..f07b3d9725 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1044,28 +1044,22 @@ virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs, if (VIR_ALLOC(addrs->zpciIds) < 0) return -1; - if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL, - virZPCIAddrKeyCode, - virZPCIAddrKeyEqual, - virZPCIAddrKeyCopy, - virZPCIAddrKeyPrintHuman, - virZPCIAddrKeyFree))) - goto error; - - if (!(addrs->zpciIds->fids = virHashCreateFull(10, NULL, - virZPCIAddrKeyCode, - virZPCIAddrKeyEqual, - virZPCIAddrKeyCopy, - virZPCIAddrKeyPrintHuman, - virZPCIAddrKeyFree))) - goto error; + addrs->zpciIds->uids = virHashCreateFull(10, NULL, + virZPCIAddrKeyCode, + virZPCIAddrKeyEqual, + virZPCIAddrKeyCopy, + virZPCIAddrKeyPrintHuman, + virZPCIAddrKeyFree); + + addrs->zpciIds->fids = virHashCreateFull(10, NULL, + virZPCIAddrKeyCode, + virZPCIAddrKeyEqual, + virZPCIAddrKeyCopy, + virZPCIAddrKeyPrintHuman, + virZPCIAddrKeyFree); } return 0; - - error: - virDomainPCIAddressSetExtensionFree(addrs); - return -1; } -- 2.24.1
There are two calls to virHashNew which check the return value. It's not necessary any more as virHashNew always returns a valid pointer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 6 +----- src/qemu/qemu_monitor_json.c | 3 +-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index b370b686f1..64c8f6cc09 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -439,15 +439,11 @@ virDomainBackupAlignDisks(virDomainBackupDefPtr def, virDomainDefPtr dom, const char *suffix) { - g_autoptr(virHashTable) disks = NULL; + g_autoptr(virHashTable) disks = virHashNew(NULL); size_t i; int ndisks; bool backup_all = false; - - if (!(disks = virHashNew(NULL))) - return -1; - /* Unlikely to have a guest without disks but technically possible. */ if (!dom->ndisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e27f3085c4..981d091ba0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2990,8 +2990,7 @@ qemuMonitorJSONBlockGetNamedNodeDataJSON(virJSONValuePtr nodes) { g_autoptr(virHashTable) ret = NULL; - if (!(ret = virHashNew((virHashDataFree) qemuMonitorJSONBlockNamedNodeDataFree))) - return NULL; + ret = virHashNew((virHashDataFree) qemuMonitorJSONBlockNamedNodeDataFree); if (virJSONValueArrayForeachSteal(nodes, qemuMonitorJSONBlockGetNamedNodeDataWorker, -- 2.24.1
Refactor the logic to skip the body of the function if there's nothing to do. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 299bf17c9e..73ae815818 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -464,8 +464,13 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, if (VIR_ALLOC(mig->nbd) < 0) return -1; - if (vm->def->ndisks && - VIR_ALLOC_N(mig->nbd->disks, vm->def->ndisks) < 0) + mig->nbd->port = priv->nbdPort; + mig->flags |= QEMU_MIGRATION_COOKIE_NBD; + + if (vm->def->ndisks == 0) + return 0; + + if (VIR_ALLOC_N(mig->nbd->disks, vm->def->ndisks) < 0) return -1; mig->nbd->ndisks = 0; @@ -496,9 +501,6 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, mig->nbd->ndisks++; } - mig->nbd->port = priv->nbdPort; - mig->flags |= QEMU_MIGRATION_COOKIE_NBD; - ret = 0; cleanup: virHashFree(stats); -- 2.24.1
Extract the struct so that it's type has a name. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.h b/src/qemu/qemu_migration_cookie.h index 20e1ed60ca..1e88684589 100644 --- a/src/qemu/qemu_migration_cookie.h +++ b/src/qemu/qemu_migration_cookie.h @@ -84,16 +84,18 @@ struct _qemuMigrationCookieNetwork { qemuMigrationCookieNetDataPtr net; }; +struct qemuMigrationCookieNBDDisk { + char *target; /* Disk target */ + unsigned long long capacity; /* And its capacity */ +}; + typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD; typedef qemuMigrationCookieNBD *qemuMigrationCookieNBDPtr; struct _qemuMigrationCookieNBD { int port; /* on which port does NBD server listen for incoming data */ size_t ndisks; /* Number of items in @disk array */ - struct { - char *target; /* Disk target */ - unsigned long long capacity; /* And its capacity */ - } *disks; + struct qemuMigrationCookieNBDDisk *disks; }; typedef struct _qemuMigrationCookieCaps qemuMigrationCookieCaps; -- 2.24.1
On 1/30/20 3:53 PM, Peter Krempa wrote:
Extract the struct so that it's type has a name.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_migration_cookie.h b/src/qemu/qemu_migration_cookie.h index 20e1ed60ca..1e88684589 100644 --- a/src/qemu/qemu_migration_cookie.h +++ b/src/qemu/qemu_migration_cookie.h @@ -84,16 +84,18 @@ struct _qemuMigrationCookieNetwork { qemuMigrationCookieNetDataPtr net; };
+struct qemuMigrationCookieNBDDisk { + char *target; /* Disk target */ + unsigned long long capacity; /* And its capacity */ +}; +
Might as well typedef this struct, so that in the next patch you can drop the 'struct '. Michal
On Thu, Jan 30, 2020 at 16:51:22 +0100, Michal Privoznik wrote:
On 1/30/20 3:53 PM, Peter Krempa wrote:
Extract the struct so that it's type has a name.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_migration_cookie.h b/src/qemu/qemu_migration_cookie.h index 20e1ed60ca..1e88684589 100644 --- a/src/qemu/qemu_migration_cookie.h +++ b/src/qemu/qemu_migration_cookie.h @@ -84,16 +84,18 @@ struct _qemuMigrationCookieNetwork { qemuMigrationCookieNetDataPtr net; };
+struct qemuMigrationCookieNBDDisk { + char *target; /* Disk target */ + unsigned long long capacity; /* And its capacity */ +}; +
Might as well typedef this struct, so that in the next patch you can drop the 'struct '.
To be honest, I'm not a fan of typedefing random internal structs and typedefing the pointer type is almost dangerous.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 73ae815818..1c3de13983 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -461,8 +461,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, /* It is not a bug if there already is a NBD data */ qemuMigrationCookieNBDFree(mig->nbd); - if (VIR_ALLOC(mig->nbd) < 0) - return -1; + mig->nbd = g_new0(qemuMigrationCookieNBD, 1); mig->nbd->port = priv->nbdPort; mig->flags |= QEMU_MIGRATION_COOKIE_NBD; @@ -470,8 +469,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, if (vm->def->ndisks == 0) return 0; - if (VIR_ALLOC_N(mig->nbd->disks, vm->def->ndisks) < 0) - return -1; + mig->nbd->disks = g_new0(struct qemuMigrationCookieNBDDisk, vm->def->ndisks); mig->nbd->ndisks = 0; for (i = 0; i < vm->def->ndisks; i++) { -- 2.24.1
On 1/30/20 3:53 PM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 73ae815818..1c3de13983 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -461,8 +461,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, /* It is not a bug if there already is a NBD data */ qemuMigrationCookieNBDFree(mig->nbd);
- if (VIR_ALLOC(mig->nbd) < 0) - return -1; + mig->nbd = g_new0(qemuMigrationCookieNBD, 1);
mig->nbd->port = priv->nbdPort; mig->flags |= QEMU_MIGRATION_COOKIE_NBD; @@ -470,8 +469,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, if (vm->def->ndisks == 0) return 0;
- if (VIR_ALLOC_N(mig->nbd->disks, vm->def->ndisks) < 0) - return -1; + mig->nbd->disks = g_new0(struct qemuMigrationCookieNBDDisk, vm->def->ndisks);
This 'struct ' should be dropped.
mig->nbd->ndisks = 0;
for (i = 0; i < vm->def->ndisks; i++) {
Michal
The data is gathered only once so we can move the whole block which fetches the data out of the loop and get rid of the logic which prevents multiple calls. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 1c3de13983..33ab6cb7a5 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -472,24 +472,19 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, mig->nbd->disks = g_new0(struct qemuMigrationCookieNBDDisk, vm->def->ndisks); mig->nbd->ndisks = 0; + if (!(stats = virHashCreate(10, virHashValueFree))) + goto cleanup; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, priv->job.asyncJob) < 0) + goto cleanup; + rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + goto cleanup; + for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; qemuBlockStats *entry; - if (!stats) { - if (!(stats = virHashCreate(10, virHashValueFree))) - goto cleanup; - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - priv->job.asyncJob) < 0) - goto cleanup; - rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; - if (rc < 0) - goto cleanup; - } - if (!disk->info.alias || !(entry = virHashLookup(stats, disk->info.alias))) continue; -- 2.24.1
Swithc to the helper which doesn't require checking of the return value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 33ab6cb7a5..968a9b589c 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -454,7 +454,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - virHashTablePtr stats = NULL; + g_autoptr(virHashTable) stats = virHashNew(virHashValueFree); size_t i; int ret = -1, rc; @@ -472,9 +472,6 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, mig->nbd->disks = g_new0(struct qemuMigrationCookieNBDDisk, vm->def->ndisks); mig->nbd->ndisks = 0; - if (!(stats = virHashCreate(10, virHashValueFree))) - goto cleanup; - if (qemuDomainObjEnterMonitorAsync(driver, vm, priv->job.asyncJob) < 0) goto cleanup; rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); @@ -496,7 +493,6 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, ret = 0; cleanup: - virHashFree(stats); return ret; } -- 2.24.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 968a9b589c..734d95f4f1 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -456,7 +456,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virHashTable) stats = virHashNew(virHashValueFree); size_t i; - int ret = -1, rc; + int rc; /* It is not a bug if there already is a NBD data */ qemuMigrationCookieNBDFree(mig->nbd); @@ -473,10 +473,10 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, mig->nbd->ndisks = 0; if (qemuDomainObjEnterMonitorAsync(driver, vm, priv->job.asyncJob) < 0) - goto cleanup; + return -1; rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - goto cleanup; + return -1; for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; @@ -491,9 +491,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, mig->nbd->ndisks++; } - ret = 0; - cleanup: - return ret; + return 0; } -- 2.24.1
With -blockdev we must look up via the nodename rather than the 'drive' alias which is not present any more. This fixes the pre-creation of storage volumes on migration with non-shared storage. https://bugzilla.redhat.com/show_bug.cgi?id=1793263 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration_cookie.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 734d95f4f1..a5a9edffc3 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -455,6 +455,7 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, { qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virHashTable) stats = virHashNew(virHashValueFree); + bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); size_t i; int rc; @@ -474,7 +475,10 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, if (qemuDomainObjEnterMonitorAsync(driver, vm, priv->job.asyncJob) < 0) return -1; - rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); + if (blockdev) + rc = qemuMonitorBlockStatsUpdateCapacityBlockdev(priv->mon, stats); + else + rc = qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats, false); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) return -1; @@ -482,9 +486,14 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig, virDomainDiskDefPtr disk = vm->def->disks[i]; qemuBlockStats *entry; - if (!disk->info.alias || - !(entry = virHashLookup(stats, disk->info.alias))) - continue; + if (blockdev) { + if (!(entry = virHashLookup(stats, disk->src->nodeformat))) + continue; + } else { + if (!disk->info.alias || + !(entry = virHashLookup(stats, disk->info.alias))) + continue; + } mig->nbd->disks[mig->nbd->ndisks].target = g_strdup(disk->dst); mig->nbd->disks[mig->nbd->ndisks].capacity = entry->capacity; -- 2.24.1
On 1/30/20 3:53 PM, Peter Krempa wrote:
The last patch fixes a bug where we'd fail to detect capacity of disks on migration source due to changes in disk topology related to blockdev.
The rest of the series makes the function at least somewhat sane.
Peter Krempa (10): util: hash: Use g_new0 for allocating hash internals conf: domain: Remove checking of return value of virHashCreateFull Remove checking of return value of virHashNew qemuMigrationCookieAddNBD: Exit early if there are no disks qemuMigrationCookieNBD: Extract embedded struct qemuMigrationCookieAddNBD: Use glib memory allocators qemuMigrationCookieAddNBD: Move monitor call out of the loop qemuMigrationCookieAddNBD: Use virHashNew and automatic freeing of virHashTablePtr qemuMigrationCookieAddNBD: Remove 'ret' variable and 'cleanup' label qemuMigrationCookieAddNBD: Fix filling of 'capacity' when blockdev is used
src/conf/backup_conf.c | 6 +--- src/conf/domain_addr.c | 32 +++++++----------- src/qemu/qemu_migration_cookie.c | 58 +++++++++++++++----------------- src/qemu/qemu_migration_cookie.h | 10 +++--- src/qemu/qemu_monitor_json.c | 3 +- src/util/virhash.c | 14 +++----- 6 files changed, 54 insertions(+), 69 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Peter Krempa