Use g_autoptr for virBitmap and get rid of the 'cleanup:' label and ret variable. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index a8d18928de..795c6f93c4 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -292,8 +292,7 @@ virDomainCheckpointCompareDiskIndex(const void *a, const void *b) int virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) { - int ret = -1; - virBitmapPtr map = NULL; + g_autoptr(virBitmap) map = NULL; size_t i; int ndisks; int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; @@ -301,13 +300,13 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) if (!def->parent.dom) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing domain in checkpoint")); - goto cleanup; + return -1; } if (def->ndisks > def->parent.dom->ndisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("too many disk checkpoint requests for domain")); - goto cleanup; + return -1; } /* Unlikely to have a guest without disks but technically possible. */ @@ -315,7 +314,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("domain must have at least one disk to perform " "checkpoints")); - goto cleanup; + return -1; } /* If <disks> omitted, do bitmap on all writeable disks; @@ -333,14 +332,14 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) if (idx < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("no disk named '%s'"), disk->name); - goto cleanup; + return -1; } if (virBitmapIsBitSet(map, idx)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' specified twice"), disk->name); - goto cleanup; + return -1; } if ((virStorageSourceIsEmpty(def->parent.dom->disks[idx]->src) || def->parent.dom->disks[idx]->src->readonly) && @@ -348,7 +347,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' is empty or readonly"), disk->name); - goto cleanup; + return -1; } ignore_value(virBitmapSetBit(map, idx)); disk->idx = idx; @@ -363,7 +362,7 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) ndisks = def->ndisks; if (VIR_EXPAND_N(def->disks, def->ndisks, def->parent.dom->ndisks - def->ndisks) < 0) - goto cleanup; + return -1; for (i = 0; i < def->parent.dom->ndisks; i++) { virDomainCheckpointDiskDefPtr disk; @@ -387,13 +386,9 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) /* Generate default bitmap names for checkpoint */ if (virDomainCheckpointDefAssignBitmapNames(def) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virBitmapFree(map); - return ret; + return 0; } -- 2.28.0

Extract the pointer and use a local variable throughout the function. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 914deb41f2..2375c78b92 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -292,25 +292,26 @@ virDomainCheckpointCompareDiskIndex(const void *a, const void *b) int virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) { + virDomainDefPtr domdef = def->parent.dom; g_autoptr(virBitmap) map = NULL; size_t i; int ndisks; int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; - if (!def->parent.dom) { + if (!domdef) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing domain in checkpoint")); return -1; } - if (def->ndisks > def->parent.dom->ndisks) { + if (def->ndisks > domdef->ndisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("too many disk checkpoint requests for domain")); return -1; } /* Unlikely to have a guest without disks but technically possible. */ - if (!def->parent.dom->ndisks) { + if (!domdef->ndisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("domain must have at least one disk to perform checkpoints")); return -1; @@ -321,12 +322,12 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) if (!def->ndisks) checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP; - map = virBitmapNew(def->parent.dom->ndisks); + map = virBitmapNew(domdef->ndisks); /* Double check requested disks. */ for (i = 0; i < def->ndisks; i++) { virDomainCheckpointDiskDefPtr disk = &def->disks[i]; - int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false); + int idx = virDomainDiskIndexByName(domdef, disk->name, false); if (idx < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -340,8 +341,8 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) disk->name); return -1; } - if ((virStorageSourceIsEmpty(def->parent.dom->disks[idx]->src) || - def->parent.dom->disks[idx]->src->readonly) && + if ((virStorageSourceIsEmpty(domdef->disks[idx]->src) || + domdef->disks[idx]->src->readonly) && disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' is empty or readonly"), @@ -351,30 +352,30 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) ignore_value(virBitmapSetBit(map, idx)); disk->idx = idx; - if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) { + if (STRNEQ(disk->name, domdef->disks[idx]->dst)) { VIR_FREE(disk->name); - disk->name = g_strdup(def->parent.dom->disks[idx]->dst); + disk->name = g_strdup(domdef->disks[idx]->dst); } } /* Provide defaults for all remaining disks. */ ndisks = def->ndisks; if (VIR_EXPAND_N(def->disks, def->ndisks, - def->parent.dom->ndisks - def->ndisks) < 0) + domdef->ndisks - def->ndisks) < 0) return -1; - for (i = 0; i < def->parent.dom->ndisks; i++) { + for (i = 0; i < domdef->ndisks; i++) { virDomainCheckpointDiskDefPtr disk; if (virBitmapIsBitSet(map, i)) continue; disk = &def->disks[ndisks++]; - disk->name = g_strdup(def->parent.dom->disks[i]->dst); + disk->name = g_strdup(domdef->disks[i]->dst); disk->idx = i; /* Don't checkpoint empty or readonly drives */ - if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src) || - def->parent.dom->disks[i]->src->readonly) + if (virStorageSourceIsEmpty(domdef->disks[i]->src) || + domdef->disks[i]->src->readonly) disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; else disk->type = checkpoint_default; -- 2.28.0

Clarify that the variable refers to the definition of the disk from the checkpoint definition. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 1881c93e09..22757d148f 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -326,35 +326,35 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) /* Double check requested disks. */ for (i = 0; i < chkdef->ndisks; i++) { - virDomainCheckpointDiskDefPtr disk = &chkdef->disks[i]; - int idx = virDomainDiskIndexByName(domdef, disk->name, false); + virDomainCheckpointDiskDefPtr chkdisk = &chkdef->disks[i]; + int idx = virDomainDiskIndexByName(domdef, chkdisk->name, false); if (idx < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("no disk named '%s'"), disk->name); + _("no disk named '%s'"), chkdisk->name); return -1; } if (virBitmapIsBitSet(map, idx)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' specified twice"), - disk->name); + chkdisk->name); return -1; } if ((virStorageSourceIsEmpty(domdef->disks[idx]->src) || domdef->disks[idx]->src->readonly) && - disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) { + chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' is empty or readonly"), - disk->name); + chkdisk->name); return -1; } ignore_value(virBitmapSetBit(map, idx)); - disk->idx = idx; + chkdisk->idx = idx; - if (STRNEQ(disk->name, domdef->disks[idx]->dst)) { - VIR_FREE(disk->name); - disk->name = g_strdup(domdef->disks[idx]->dst); + if (STRNEQ(chkdisk->name, domdef->disks[idx]->dst)) { + VIR_FREE(chkdisk->name); + chkdisk->name = g_strdup(domdef->disks[idx]->dst); } } @@ -365,20 +365,20 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) return -1; for (i = 0; i < domdef->ndisks; i++) { - virDomainCheckpointDiskDefPtr disk; + virDomainCheckpointDiskDefPtr chkdisk; if (virBitmapIsBitSet(map, i)) continue; - disk = &chkdef->disks[ndisks++]; - disk->name = g_strdup(domdef->disks[i]->dst); - disk->idx = i; + chkdisk = &chkdef->disks[ndisks++]; + chkdisk->name = g_strdup(domdef->disks[i]->dst); + chkdisk->idx = i; /* Don't checkpoint empty or readonly drives */ if (virStorageSourceIsEmpty(domdef->disks[i]->src) || domdef->disks[i]->src->readonly) - disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; + chkdisk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; else - disk->type = checkpoint_default; + chkdisk->type = checkpoint_default; } qsort(&chkdef->disks[0], chkdef->ndisks, sizeof(chkdef->disks[0]), -- 2.28.0

Similarly to d3c029bb105 where we've refactored virDomainSnapshotAlignDisks, modify the extension algorithm to avoid use of the 'idx' variable and sorting of the array. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 53 +++++++++++++++----------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 3213097f4f..bd0a673757 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -274,16 +274,6 @@ virDomainCheckpointDefAssignBitmapNames(virDomainCheckpointDefPtr def) } -static int -virDomainCheckpointCompareDiskIndex(const void *a, const void *b) -{ - const virDomainCheckpointDiskDef *diska = a; - const virDomainCheckpointDiskDef *diskb = b; - - /* Integer overflow shouldn't be a problem here. */ - return diska->idx - diskb->idx; -} - /* Align def->disks to def->domain. Sort the list of def->disks, * filling in any missing disks with appropriate default. Convert * paths to disk targets for uniformity. Issue an error and return -1 @@ -293,9 +283,9 @@ int virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) { virDomainDefPtr domdef = chkdef->parent.dom; - g_autoptr(virBitmap) map = NULL; + g_autoptr(GHashTable) map = virHashNew(NULL); + g_autofree virDomainCheckpointDiskDefPtr olddisks = NULL; size_t i; - int ndisks; int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; if (!domdef) { @@ -322,8 +312,6 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) if (!chkdef->ndisks) checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP; - map = virBitmapNew(domdef->ndisks); - /* Double check requested disks. */ for (i = 0; i < chkdef->ndisks; i++) { virDomainCheckpointDiskDefPtr chkdisk = &chkdef->disks[i]; @@ -338,13 +326,16 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) domdisk = domdef->disks[idx]; - if (virBitmapIsBitSet(map, idx)) { + if (virHashHasEntry(map, domdisk->dst)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' specified twice"), chkdisk->name); return -1; } + if (virHashAddEntry(map, domdisk->dst, chkdisk) < 0) + return -1; + if ((virStorageSourceIsEmpty(domdisk->src) || domdisk->src->readonly) && chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) { @@ -353,8 +344,6 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) chkdisk->name); return -1; } - ignore_value(virBitmapSetBit(map, idx)); - chkdisk->idx = idx; if (STRNEQ(chkdisk->name, domdisk->dst)) { VIR_FREE(chkdisk->name); @@ -362,32 +351,32 @@ virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr chkdef) } } - /* Provide defaults for all remaining disks. */ - ndisks = chkdef->ndisks; - if (VIR_EXPAND_N(chkdef->disks, chkdef->ndisks, - domdef->ndisks - chkdef->ndisks) < 0) - return -1; + olddisks = g_steal_pointer(&chkdef->disks); + chkdef->disks = g_new0(virDomainCheckpointDiskDef, domdef->ndisks); + chkdef->ndisks = domdef->ndisks; for (i = 0; i < domdef->ndisks; i++) { - virDomainCheckpointDiskDefPtr chkdisk; + virDomainDiskDefPtr domdisk = domdef->disks[i]; + virDomainCheckpointDiskDefPtr chkdisk = chkdef->disks + i; + virDomainCheckpointDiskDefPtr existing; - if (virBitmapIsBitSet(map, i)) + /* copy existing disks */ + if ((existing = virHashLookup(map, domdisk->dst))) { + memcpy(chkdisk, existing, sizeof(*chkdisk)); continue; - chkdisk = &chkdef->disks[ndisks++]; - chkdisk->name = g_strdup(domdef->disks[i]->dst); - chkdisk->idx = i; + } + + /* Provide defaults for all remaining disks. */ + chkdisk->name = g_strdup(domdisk->dst); /* Don't checkpoint empty or readonly drives */ - if (virStorageSourceIsEmpty(domdef->disks[i]->src) || - domdef->disks[i]->src->readonly) + if (virStorageSourceIsEmpty(domdisk->src) || + domdisk->src->readonly) chkdisk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; else chkdisk->type = checkpoint_default; } - qsort(&chkdef->disks[0], chkdef->ndisks, sizeof(chkdef->disks[0]), - virDomainCheckpointCompareDiskIndex); - /* Generate default bitmap names for checkpoint */ if (virDomainCheckpointDefAssignBitmapNames(chkdef) < 0) return -1; -- 2.28.0

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 49b68c2c68..6922ebf407 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17649,7 +17649,11 @@ virDomainDiskByName(virDomainDefPtr def, bool allow_ambiguous) { int idx = virDomainDiskIndexByName(def, name, allow_ambiguous); - return idx < 0 ? NULL : def->disks[idx]; + + if (idx < 0) + return NULL; + + return def->disks[idx]; } -- 2.28.0

We don't need the index that virDomainDiskIndexByName returns. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 8ef9708c72..f896fd1cf2 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -663,17 +663,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef, /* Double check requested disks. */ for (i = 0; i < snapdef->ndisks; i++) { virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i]; - int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false); - virDomainDiskDefPtr domdisk = NULL; + virDomainDiskDefPtr domdisk = virDomainDiskByName(domdef, snapdisk->name, false); - if (idx < 0) { + if (!domdisk) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("no disk named '%s'"), snapdisk->name); return -1; } - domdisk = domdef->disks[idx]; - if (virHashHasEntry(map, domdisk->dst)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' specified twice"), -- 2.28.0