8:33 a.m.
A set of patches that prevent failures from virStorageSourceNew and
refactor virDomainSnapshotAlignDisks for better readability.
Peter Krempa (13):
virStorageSourceNew: Abort on failure
virStorageVolDefParseXML: Use g_steal_pointer
qemuDomainBlockRebase: Replace ternary operator with if/else
qemuSnapshotCreateInactiveExternal: Don't access 'idx' of snapshot
virDomainSnapshotAlignDisks: Refactor cleanup
virDomainSnapshotAlignDisks: Rename 'def' -> 'snapdef'
virDomainSnapshotAlignDisks: Rename 'disk' -> 'snapdisk'
virDomainSnapshotAlignDisks: Add 'domdef' local variable
virDomainSnapshotAlignDisks: Extract domain disk definition to a local
variable
virDomainSnapshotAlignDisks: remove unnecessary 'tmp' variable
virDomainSnapshotAlignDisks: clarify handing of snapshot location
virDomainSnapshotAlignDisks: refactor extension to all disks
virDomainSnapshotDiskDef: Remove 'idx' field
src/conf/backup_conf.c | 7 +-
src/conf/domain_conf.c | 21 ++--
src/conf/snapshot_conf.c | 160 +++++++++++---------------
src/conf/snapshot_conf.h | 1 -
src/conf/storage_conf.c | 8 +-
src/qemu/qemu_domain.c | 21 ++--
src/qemu/qemu_driver.c | 18 ++-
src/qemu/qemu_migration.c | 7 +-
src/qemu/qemu_snapshot.c | 7 +-
src/storage/storage_backend_gluster.c | 5 +-
src/storage/storage_backend_logical.c | 4 +-
src/storage/storage_util.c | 10 +-
src/util/virstoragefile.c | 32 ++----
tests/qemublocktest.c | 16 +--
tests/virstoragetest.c | 5 +-
15 files changed, 121 insertions(+), 201 deletions(-)
--
2.26.2
8:33 a.m.
New subject: [PATCH 01/13] virStorageSourceNew: Abort on failure
Add an abort() on the class/object allocation failures so that
virStorageSourceNew() always returns a virStorageSource and remove
checks from all callers.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/backup_conf.c | 7 ++----
src/conf/domain_conf.c | 21 ++++++------------
src/conf/snapshot_conf.c | 7 ++----
src/conf/storage_conf.c | 4 +---
src/qemu/qemu_domain.c | 21 ++++++------------
src/qemu/qemu_driver.c | 12 +++-------
src/qemu/qemu_migration.c | 7 ++----
src/qemu/qemu_snapshot.c | 3 +--
src/storage/storage_backend_gluster.c | 5 +----
src/storage/storage_backend_logical.c | 4 +---
src/storage/storage_util.c | 10 +++------
src/util/virstoragefile.c | 32 ++++++++++-----------------
tests/qemublocktest.c | 16 +++-----------
tests/virstoragetest.c | 5 +----
14 files changed, 46 insertions(+), 108 deletions(-)
diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c
index 5caba621d8..5c475239ad 100644
--- a/src/conf/backup_conf.c
+++ b/src/conf/backup_conf.c
@@ -169,8 +169,7 @@ virDomainBackupDiskDefParseXML(xmlNodePtr node,
def->state = tmp;
}
- if (!(def->store = virStorageSourceNew()))
- return -1;
+ def->store = virStorageSourceNew();
if ((type = virXMLPropString(node, "type"))) {
if ((def->store->type = virStorageTypeFromString(type)) <= 0) {
@@ -500,9 +499,7 @@ virDomainBackupDefAssignStore(virDomainBackupDiskDefPtr disk,
}
} else if (!disk->store) {
if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_FILE) {
- if (!(disk->store = virStorageSourceNew()))
- return -1;
-
+ disk->store = virStorageSourceNew();
disk->store->type = VIR_STORAGE_TYPE_FILE;
disk->store->path = g_strdup_printf("%s.%s", src->path,
suffix);
} else {
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9289c147fe..bbe3cddadf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2187,8 +2187,7 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt)
if (VIR_ALLOC(ret) < 0)
return NULL;
- if (!(ret->src = virStorageSourceNew()))
- goto error;
+ ret->src = virStorageSourceNew();
if (xmlopt &&
xmlopt->privateData.diskNew &&
@@ -2400,8 +2399,7 @@ virDomainFSDefNew(virDomainXMLOptionPtr xmlopt)
if (VIR_ALLOC(ret) < 0)
return NULL;
- if (!(ret->src = virStorageSourceNew()))
- goto cleanup;
+ ret->src = virStorageSourceNew();
if (xmlopt &&
xmlopt->privateData.fsNew &&
@@ -8325,9 +8323,8 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr sourcenode,
if (flags & VIR_DOMAIN_DEF_PARSE_STATUS &&
xmlopt && xmlopt->privateData.storageParse) {
if ((ctxt->node = virXPathNode("./privateData", ctxt))) {
- if (!scsihostsrc->src &&
- !(scsihostsrc->src = virStorageSourceNew()))
- return -1;
+ if (!scsihostsrc->src)
+ scsihostsrc->src = virStorageSourceNew();
if (xmlopt->privateData.storageParse(ctxt, scsihostsrc->src) < 0)
return -1;
}
@@ -8353,8 +8350,7 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
/* For the purposes of command line creation, this needs to look
* like a disk storage source */
- if (!(iscsisrc->src = virStorageSourceNew()))
- return -1;
+ iscsisrc->src = virStorageSourceNew();
iscsisrc->src->type = VIR_STORAGE_TYPE_NETWORK;
iscsisrc->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
@@ -9770,9 +9766,7 @@ virDomainStorageSourceParseBase(const char *type,
{
g_autoptr(virStorageSource) src = NULL;
- if (!(src = virStorageSourceNew()))
- return NULL;
-
+ src = virStorageSourceNew();
src->type = VIR_STORAGE_TYPE_FILE;
if (type &&
@@ -9960,8 +9954,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
/* terminator does not have a type */
if (!(type = virXMLPropString(ctxt->node, "type"))) {
- if (!(src->backingStore = virStorageSourceNew()))
- return -1;
+ src->backingStore = virStorageSourceNew();
return 0;
}
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 1ee63b9141..87a00082ef 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -148,9 +148,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
ctxt->node = node;
- if (!(def->src = virStorageSourceNew()))
- goto cleanup;
-
+ def->src = virStorageSourceNew();
def->name = virXMLPropString(node, "name");
if (!def->name) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -744,8 +742,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
if (virBitmapIsBitSet(map, i))
continue;
disk = &def->disks[ndisks++];
- if (!(disk->src = virStorageSourceNew()))
- goto cleanup;
+ disk->src = virStorageSourceNew();
disk->name = g_strdup(def->parent.dom->disks[i]->dst);
disk->idx = i;
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 001f4f2bdd..2b00f09d73 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1350,9 +1350,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
}
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt)))
{
- if (!(def->target.backingStore = virStorageSourceNew()))
- return NULL;
-
+ def->target.backingStore = virStorageSourceNew();
def->target.backingStore->type = VIR_STORAGE_TYPE_FILE;
def->target.backingStore->path = backingStore;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9e0d3a15b2..c7a3e4f7cc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5273,9 +5273,8 @@
qemuDomainDeviceHostdevDefPostParseRestoreBackendAlias(virDomainHostdevDefPtr ho
switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
- if (!scsisrc->u.host.src &&
- !(scsisrc->u.host.src = virStorageSourceNew()))
- return -1;
+ if (!scsisrc->u.host.src)
+ scsisrc->u.host.src = virStorageSourceNew();
src = scsisrc->u.host.src;
break;
@@ -7167,9 +7166,8 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
}
/* terminate the chain for such images as the code below would do */
- if (!disksrc->backingStore &&
- !(disksrc->backingStore = virStorageSourceNew()))
- return -1;
+ if (!disksrc->backingStore)
+ disksrc->backingStore = virStorageSourceNew();
/* host cdrom requires special treatment in qemu, so we need to check
* whether a block device is a cdrom */
@@ -10456,8 +10454,7 @@ qemuDomainPrepareHostdev(virDomainHostdevDefPtr hostdev,
switch ((virDomainHostdevSCSIProtocolType) scsisrc->protocol) {
case VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_NONE:
virObjectUnref(scsisrc->u.host.src);
- if (!(scsisrc->u.host.src = virStorageSourceNew()))
- return -1;
+ scsisrc->u.host.src = virStorageSourceNew();
src = scsisrc->u.host.src;
break;
@@ -10850,9 +10847,7 @@ qemuDomainInitializePflashStorageSource(virDomainObjPtr vm)
if (!virDomainDefHasOldStyleUEFI(def))
return 0;
- if (!(pflash0 = virStorageSourceNew()))
- return -1;
-
+ pflash0 = virStorageSourceNew();
pflash0->type = VIR_STORAGE_TYPE_FILE;
pflash0->format = VIR_STORAGE_FILE_RAW;
pflash0->path = g_strdup(def->os.loader->path);
@@ -10862,9 +10857,7 @@ qemuDomainInitializePflashStorageSource(virDomainObjPtr vm)
if (def->os.loader->nvram) {
- if (!(pflash1 = virStorageSourceNew()))
- return -1;
-
+ pflash1 = virStorageSourceNew();
pflash1->type = VIR_STORAGE_TYPE_FILE;
pflash1->format = VIR_STORAGE_FILE_RAW;
pflash1->path = g_strdup(def->os.loader->nvram);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae715c01d7..9b5ff3a65c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14895,10 +14895,8 @@
qemuDomainBlockCopyCommonValidateUserMirrorBackingStore(virStorageSourcePtr mirr
{
if (!virStorageSourceHasBacking(mirror)) {
/* for deep copy there won't be backing chain so we can terminate it */
- if (!mirror->backingStore &&
- !shallow &&
- !(mirror->backingStore = virStorageSourceNew()))
- return -1;
+ if (!mirror->backingStore && !shallow)
+ mirror->backingStore = virStorageSourceNew();
/* When reusing an external image we document that the user must ensure
* that the <mirror> image must expose data as the original image did
@@ -15149,9 +15147,6 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY)) {
g_autoptr(virStorageSource) terminator = virStorageSourceNew();
- if (!terminator)
- goto endjob;
-
if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdevTop(mirror,
terminator,
priv->qemuCaps)))
@@ -15296,8 +15291,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const
char *base,
return qemuDomainBlockPullCommon(vm, path, base, bandwidth, flags);
/* If we got here, we are doing a block copy rebase. */
- if (!(dest = virStorageSourceNew()))
- goto cleanup;
+ dest = virStorageSourceNew();
dest->type = (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) ?
VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE;
dest->path = g_strdup(base);
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a530c17582..5708334d2f 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -837,15 +837,12 @@
qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDefPtr disk,
{
g_autoptr(virStorageSource) copysrc = NULL;
- if (!(copysrc = virStorageSourceNew()))
- return NULL;
-
+ copysrc = virStorageSourceNew();
copysrc->type = VIR_STORAGE_TYPE_NETWORK;
copysrc->protocol = VIR_STORAGE_NET_PROTOCOL_NBD;
copysrc->format = VIR_STORAGE_FILE_RAW;
- if (!(copysrc->backingStore = virStorageSourceNew()))
- return NULL;
+ copysrc->backingStore = virStorageSourceNew();
if (!(copysrc->path = qemuAliasDiskDriveFromDisk(disk)))
return NULL;
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 5f49fd17a4..a6e091dd09 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -894,8 +894,7 @@ qemuSnapshotDiskPrepareOneBlockdev(virQEMUDriverPtr driver,
/* create a terminator for the snapshot disks so that qemu does not try
* to open them at first */
- if (!(terminator = virStorageSourceNew()))
- return -1;
+ terminator = virStorageSourceNew();
if (qemuDomainPrepareStorageSourceBlockdev(dd->disk, dd->src,
priv, cfg) < 0)
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index 70e6f2b086..4763a569e3 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -279,11 +279,8 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
goto cleanup;
if (meta->backingStoreRaw) {
- if (!(vol->target.backingStore = virStorageSourceNew()))
- goto cleanup;
-
+ vol->target.backingStore = virStorageSourceNew();
vol->target.backingStore->type = VIR_STORAGE_TYPE_NETWORK;
-
vol->target.backingStore->path =
g_steal_pointer(&meta->backingStoreRaw);
vol->target.backingStore->format = meta->backingStoreRawFormat;
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 43cf3a1598..e3debc390c 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -282,9 +282,7 @@ virStorageBackendLogicalMakeVol(char **const groups,
* lv is created with "--virtualsize").
*/
if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] !=
'[')) {
- if (!(vol->target.backingStore = virStorageSourceNew()))
- goto cleanup;
-
+ vol->target.backingStore = virStorageSourceNew();
vol->target.backingStore->path = g_strdup_printf("%s/%s",
def->target.path,
groups[1]);
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 9171cb084f..08d9bd4172 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -3389,8 +3389,7 @@ storageBackendProbeTarget(virStorageSourcePtr target,
return -1;
if (meta->backingStoreRaw) {
- if (virStorageSourceNewFromBacking(meta, &target->backingStore) < 0)
- return -1;
+ virStorageSourceNewFromBacking(meta, &target->backingStore);
/* XXX: Remote storage doesn't play nicely with volumes backed by
* remote storage. To avoid trouble, just fake the backing store is RAW
@@ -3398,9 +3397,7 @@ storageBackendProbeTarget(virStorageSourcePtr target,
if (!virStorageSourceIsLocalStorage(target->backingStore)) {
virObjectUnref(target->backingStore);
- if (!(target->backingStore = virStorageSourceNew()))
- return -1;
-
+ target->backingStore = virStorageSourceNew();
target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
target->backingStore->path = meta->backingStoreRaw;
meta->backingStoreRaw = NULL;
@@ -3568,8 +3565,7 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
goto cleanup;
VIR_DIR_CLOSE(dir);
- if (!(target = virStorageSourceNew()))
- goto cleanup;
+ target = virStorageSourceNew();
if ((fd = open(def->target.path, O_RDONLY)) < 0) {
virReportSystemError(errno,
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 42341150e5..229de14935 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1084,10 +1084,7 @@ static virStorageSourcePtr
virStorageFileMetadataNew(const char *path,
int format)
{
- g_autoptr(virStorageSource) def = NULL;
-
- if (!(def = virStorageSourceNew()))
- return NULL;
+ g_autoptr(virStorageSource) def = virStorageSourceNew();
def->format = format;
def->type = VIR_STORAGE_TYPE_FILE;
@@ -2368,10 +2365,7 @@ virStorageSourcePtr
virStorageSourceCopy(const virStorageSource *src,
bool backingChain)
{
- g_autoptr(virStorageSource) def = NULL;
-
- if (!(def = virStorageSourceNew()))
- return NULL;
+ g_autoptr(virStorageSource) def = virStorageSourceNew();
def->id = src->id;
def->type = src->type;
@@ -2746,10 +2740,15 @@ VIR_ONCE_GLOBAL_INIT(virStorageSource);
virStorageSourcePtr
virStorageSourceNew(void)
{
+ virStorageSourcePtr ret;
+
if (virStorageSourceInitialize() < 0)
- return NULL;
+ abort();
+
+ if (!(ret = virObjectNew(virStorageSourceClass)))
+ abort();
- return virObjectNew(virStorageSourceClass);
+ return ret;
}
@@ -2758,10 +2757,7 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
const char *rel)
{
g_autofree char *dirname = NULL;
- g_autoptr(virStorageSource) def = NULL;
-
- if (!(def = virStorageSourceNew()))
- return NULL;
+ g_autoptr(virStorageSource) def = virStorageSourceNew();
/* store relative name */
def->relPath = g_strdup(rel);
@@ -3980,13 +3976,10 @@ virStorageSourceNewFromBackingAbsolute(const char *path,
const char *json;
const char *dirpath;
int rc = 0;
- g_autoptr(virStorageSource) def = NULL;
+ g_autoptr(virStorageSource) def = virStorageSourceNew();
*src = NULL;
- if (!(def = virStorageSourceNew()))
- return -1;
-
if (virStorageIsFile(path)) {
def->type = VIR_STORAGE_TYPE_FILE;
@@ -5317,8 +5310,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
src->backingStore = g_steal_pointer(&backingStore);
} else {
/* add terminator */
- if (!(src->backingStore = virStorageSourceNew()))
- return -1;
+ src->backingStore = virStorageSourceNew();
}
return 0;
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 0685b703a1..7bde613843 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -67,9 +67,7 @@ testBackingXMLjsonXML(const void *args)
if (data->legacy)
backendpropsflags |= QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY;
- if (!(xmlsrc = virStorageSourceNew()))
- return -1;
-
+ xmlsrc = virStorageSourceNew();
xmlsrc->type = data->type;
if (!(xml = virXMLParseStringCtxt(data->xml, "(test storage source
XML)", &ctxt)))
@@ -673,10 +671,7 @@ testQemuBitmapListPrint(const char *title,
static virStorageSourcePtr
testQemuBackupIncrementalBitmapCalculateGetFakeImage(size_t idx)
{
- virStorageSourcePtr ret;
-
- if (!(ret = virStorageSourceNew()))
- abort();
+ virStorageSourcePtr ret = virStorageSourceNew();
ret->id = idx;
ret->type = VIR_STORAGE_TYPE_FILE;
@@ -755,9 +750,7 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque)
return -1;
}
- if (!(target = virStorageSourceNew()))
- return -1;
-
+ target = virStorageSourceNew();
target->nodeformat = g_strdup_printf("target_node");
if (qemuBackupDiskPrepareOneBitmapsChain(data->chain,
@@ -889,9 +882,6 @@ testQemuBlockBitmapBlockcopy(const void *opaque)
g_autoptr(virStorageSource) fakemirror = virStorageSourceNew();
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- if (!fakemirror)
- return -1;
-
fakemirror->nodeformat = g_strdup("mirror-format-node");
expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir,
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index e0f8b6d621..2e466ecb99 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -85,10 +85,7 @@ testStorageFileGetMetadata(const char *path,
uid_t uid, gid_t gid)
{
struct stat st;
- g_autoptr(virStorageSource) def = NULL;
-
- if (!(def = virStorageSourceNew()))
- return NULL;
+ g_autoptr(virStorageSource) def = virStorageSourceNew();
def->type = VIR_STORAGE_TYPE_FILE;
def->format = format;
--
2.26.2
10:24 a.m.
New subject: [PATCH 01/13] virStorageSourceNew: Abort on failure
On a Wednesday in 2020, Peter Krempa wrote:
Add an abort() on the class/object allocation failures so that
virStorageSourceNew() always returns a virStorageSource and remove
checks from all callers.
The allocation APIs in GLib and libvirt already do abort().
All that's left are usage errors.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/backup_conf.c | 7 ++----
src/conf/domain_conf.c | 21 ++++++------------
src/conf/snapshot_conf.c | 7 ++----
src/conf/storage_conf.c | 4 +---
src/qemu/qemu_domain.c | 21 ++++++------------
src/qemu/qemu_driver.c | 12 +++-------
src/qemu/qemu_migration.c | 7 ++----
src/qemu/qemu_snapshot.c | 3 +--
src/storage/storage_backend_gluster.c | 5 +----
src/storage/storage_backend_logical.c | 4 +---
src/storage/storage_util.c | 10 +++------
src/util/virstoragefile.c | 32 ++++++++++-----------------
tests/qemublocktest.c | 16 +++-----------
tests/virstoragetest.c | 5 +----
14 files changed, 46 insertions(+), 108 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 02/13] virStorageVolDefParseXML: Use g_steal_pointer
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/storage_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 2b00f09d73..d53d50479b 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1352,9 +1352,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt)))
{
def->target.backingStore = virStorageSourceNew();
def->target.backingStore->type = VIR_STORAGE_TYPE_FILE;
-
- def->target.backingStore->path = backingStore;
- backingStore = NULL;
+ def->target.backingStore->path = g_steal_pointer(&backingStore);
if (options->formatFromString) {
g_autofree char *format = NULL;
--
2.26.2
10:26 a.m.
New subject: [PATCH 02/13] virStorageVolDefParseXML: Use g_steal_pointer
On a Wednesday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/storage_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 03/13] qemuDomainBlockRebase: Replace ternary operator with if/else
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9b5ff3a65c..40d11aced6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15292,8 +15292,10 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const
char *base,
/* If we got here, we are doing a block copy rebase. */
dest = virStorageSourceNew();
- dest->type = (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) ?
- VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE;
+ if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)
+ dest->type = VIR_STORAGE_TYPE_BLOCK;
+ else
+ dest->type = VIR_STORAGE_TYPE_FILE;
dest->path = g_strdup(base);
if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)
dest->format = VIR_STORAGE_FILE_RAW;
--
2.26.2
10:27 a.m.
New subject: [PATCH 03/13] qemuDomainBlockRebase: Replace ternary operator with if/else
On a Wednesday in 2020, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 04/13] qemuSnapshotCreateInactiveExternal: Don't access 'idx' of snapshot
After virDomainSnapshotAlignDisks is called the definitions of disks in
the snapshot definition and in the domain definition are in the same
order so they can be addressed using the same index.
This frees up 'idx' to be removed later.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index a6e091dd09..48a4e9dd41 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -171,7 +171,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
* create them correctly. */
for (i = 0; i < snapdef->ndisks && !reuse; i++) {
snapdisk = &(snapdef->disks[i]);
- defdisk = snapdef->parent.dom->disks[snapdisk->idx];
+ defdisk = vm->def->disks[i];
if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
continue;
@@ -216,7 +216,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
g_autoptr(virStorageSource) newsrc = NULL;
snapdisk = &(snapdef->disks[i]);
- defdisk = vm->def->disks[snapdisk->idx];
+ defdisk = vm->def->disks[i];
if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
continue;
--
2.26.2
10:29 a.m.
New subject: [PATCH 04/13] qemuSnapshotCreateInactiveExternal: Don't access 'idx' of snapshot
On a Wednesday in 2020, Peter Krempa wrote:
After virDomainSnapshotAlignDisks is called the definitions of disks
in
the snapshot definition and in the domain definition are in the same
order so they can be addressed using the same index.
This frees up 'idx' to be removed later.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 05/13] virDomainSnapshotAlignDisks: Refactor cleanup
Use automatic pointer for the bitmap and get rid of the 'cleanup' label
and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 87a00082ef..160f2054a4 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -649,31 +649,28 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
int default_snapshot,
bool require_match)
{
- int ret = -1;
- virBitmapPtr map = NULL;
+ g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
if (!def->parent.dom) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in snapshot"));
- goto cleanup;
+ return -1;
}
if (def->ndisks > def->parent.dom->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk snapshot requests for domain"));
- goto cleanup;
+ return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
- if (!def->parent.dom->ndisks) {
- ret = 0;
- goto cleanup;
- }
+ if (!def->parent.dom->ndisks)
+ return 0;
if (!(map = virBitmapNew(def->parent.dom->ndisks)))
- goto cleanup;
+ return -1;
/* Double check requested disks. */
for (i = 0; i < def->ndisks; i++) {
@@ -684,14 +681,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr 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;
}
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
@@ -714,7 +711,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode
'%s'"),
disk->name, tmp);
- goto cleanup;
+ return -1;
}
if (disk->src->path &&
disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
@@ -722,7 +719,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
_("file '%s' for disk '%s' requires
"
"use of external snapshot mode"),
disk->src->path, disk->name);
- goto cleanup;
+ return -1;
}
if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
VIR_FREE(disk->name);
@@ -734,7 +731,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr 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++) {
virDomainSnapshotDiskDefPtr disk;
@@ -762,13 +759,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
/* Generate default external file names for external snapshot locations */
if (virDomainSnapshotDefAssignExternalNames(def) < 0)
- goto cleanup;
-
- ret = 0;
+ return -1;
- cleanup:
- virBitmapFree(map);
- return ret;
+ return 0;
}
--
2.26.2
10:30 a.m.
New subject: [PATCH 05/13] virDomainSnapshotAlignDisks: Refactor cleanup
On a Wednesday in 2020, Peter Krempa wrote:
Use automatic pointer for the bitmap and get rid of the
'cleanup' label
and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 06/13] virDomainSnapshotAlignDisks: Rename 'def' -> 'snapdef'
While this function resides in the snapshot config module the 'def'
variable is referencing the VM definition in most places. Change the
name to 'snapdef' to avoid ambiguity especially since we are also
dealing with the domain definition in this function.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 42 ++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 160f2054a4..77f53c4a1d 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -645,7 +645,7 @@ virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
* dom->disks. If require_match, also ensure that there is no
* conflicting requests for both internal and external snapshots. */
int
-virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
+virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
int default_snapshot,
bool require_match)
{
@@ -653,29 +653,29 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
size_t i;
int ndisks;
- if (!def->parent.dom) {
+ if (!snapdef->parent.dom) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in snapshot"));
return -1;
}
- if (def->ndisks > def->parent.dom->ndisks) {
+ if (snapdef->ndisks > snapdef->parent.dom->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk snapshot requests for domain"));
return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
- if (!def->parent.dom->ndisks)
+ if (!snapdef->parent.dom->ndisks)
return 0;
- if (!(map = virBitmapNew(def->parent.dom->ndisks)))
+ if (!(map = virBitmapNew(snapdef->parent.dom->ndisks)))
return -1;
/* Double check requested disks. */
- for (i = 0; i < def->ndisks; i++) {
- virDomainSnapshotDiskDefPtr disk = &def->disks[i];
- int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false);
+ for (i = 0; i < snapdef->ndisks; i++) {
+ virDomainSnapshotDiskDefPtr disk = &snapdef->disks[i];
+ int idx = virDomainDiskIndexByName(snapdef->parent.dom, disk->name,
false);
int disk_snapshot;
if (idx < 0) {
@@ -693,7 +693,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
- disk_snapshot = def->parent.dom->disks[idx]->snapshot;
+ disk_snapshot = snapdef->parent.dom->disks[idx]->snapshot;
if (!disk->snapshot) {
if (disk_snapshot &&
(!require_match ||
@@ -721,44 +721,44 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
disk->src->path, disk->name);
return -1;
}
- if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
+ if (STRNEQ(disk->name, snapdef->parent.dom->disks[idx]->dst)) {
VIR_FREE(disk->name);
- disk->name = g_strdup(def->parent.dom->disks[idx]->dst);
+ disk->name = g_strdup(snapdef->parent.dom->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)
+ ndisks = snapdef->ndisks;
+ if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
+ snapdef->parent.dom->ndisks - snapdef->ndisks) < 0)
return -1;
- for (i = 0; i < def->parent.dom->ndisks; i++) {
+ for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
virDomainSnapshotDiskDefPtr disk;
if (virBitmapIsBitSet(map, i))
continue;
- disk = &def->disks[ndisks++];
+ disk = &snapdef->disks[ndisks++];
disk->src = virStorageSourceNew();
- disk->name = g_strdup(def->parent.dom->disks[i]->dst);
+ disk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
disk->idx = i;
/* Don't snapshot empty drives */
- if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src))
+ if (virStorageSourceIsEmpty(snapdef->parent.dom->disks[i]->src))
disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
else
- disk->snapshot = def->parent.dom->disks[i]->snapshot;
+ disk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
disk->src->type = VIR_STORAGE_TYPE_FILE;
if (!disk->snapshot)
disk->snapshot = default_snapshot;
}
- qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
+ qsort(&snapdef->disks[0], snapdef->ndisks, sizeof(snapdef->disks[0]),
virDomainSnapshotCompareDiskIndex);
/* Generate default external file names for external snapshot locations */
- if (virDomainSnapshotDefAssignExternalNames(def) < 0)
+ if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0)
return -1;
return 0;
--
2.26.2
10:32 a.m.
New subject: [PATCH 06/13] virDomainSnapshotAlignDisks: Rename 'def' -> 'snapdef'
On a Wednesday in 2020, Peter Krempa wrote:
While this function resides in the snapshot config module the
'def'
, the
variable is referencing the VM definition in most places. Change the
name to 'snapdef' to avoid ambiguity especially since we are also
dealing with the domain definition in this function.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 42 ++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 21 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 07/13] virDomainSnapshotAlignDisks: Rename 'disk' -> 'snapdisk'
The 'disk' variable usually refers to a definition of a disk from the
domain definition. Rename it to 'snapdisk' to be clear that we are
talking about the snapshot disk definition especially since this
function also accesses the domain disk definition.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 54 ++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 77f53c4a1d..c835ec7333 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -674,56 +674,56 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
- virDomainSnapshotDiskDefPtr disk = &snapdef->disks[i];
- int idx = virDomainDiskIndexByName(snapdef->parent.dom, disk->name,
false);
+ virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i];
+ int idx = virDomainDiskIndexByName(snapdef->parent.dom, snapdisk->name,
false);
int disk_snapshot;
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("no disk named '%s'"), disk->name);
+ _("no disk named '%s'"),
snapdisk->name);
return -1;
}
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
- disk->name);
+ snapdisk->name);
return -1;
}
ignore_value(virBitmapSetBit(map, idx));
- disk->idx = idx;
+ snapdisk->idx = idx;
disk_snapshot = snapdef->parent.dom->disks[idx]->snapshot;
- if (!disk->snapshot) {
+ if (!snapdisk->snapshot) {
if (disk_snapshot &&
(!require_match ||
disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE))
- disk->snapshot = disk_snapshot;
+ snapdisk->snapshot = disk_snapshot;
else
- disk->snapshot = default_snapshot;
+ snapdisk->snapshot = default_snapshot;
} else if (require_match &&
- disk->snapshot != default_snapshot &&
- !(disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
+ snapdisk->snapshot != default_snapshot &&
+ !(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE
&&
disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
const char *tmp;
tmp = virDomainSnapshotLocationTypeToString(default_snapshot);
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode
'%s'"),
- disk->name, tmp);
+ snapdisk->name, tmp);
return -1;
}
- if (disk->src->path &&
- disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ if (snapdisk->src->path &&
+ snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("file '%s' for disk '%s' requires
"
"use of external snapshot mode"),
- disk->src->path, disk->name);
+ snapdisk->src->path, snapdisk->name);
return -1;
}
- if (STRNEQ(disk->name, snapdef->parent.dom->disks[idx]->dst)) {
- VIR_FREE(disk->name);
- disk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
+ if (STRNEQ(snapdisk->name, snapdef->parent.dom->disks[idx]->dst)) {
+ VIR_FREE(snapdisk->name);
+ snapdisk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
}
}
@@ -734,24 +734,24 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
return -1;
for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
- virDomainSnapshotDiskDefPtr disk;
+ virDomainSnapshotDiskDefPtr snapdisk;
if (virBitmapIsBitSet(map, i))
continue;
- disk = &snapdef->disks[ndisks++];
- disk->src = virStorageSourceNew();
- disk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
- disk->idx = i;
+ snapdisk = &snapdef->disks[ndisks++];
+ snapdisk->src = virStorageSourceNew();
+ snapdisk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
+ snapdisk->idx = i;
/* Don't snapshot empty drives */
if (virStorageSourceIsEmpty(snapdef->parent.dom->disks[i]->src))
- disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
+ snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
else
- disk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
+ snapdisk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
- disk->src->type = VIR_STORAGE_TYPE_FILE;
- if (!disk->snapshot)
- disk->snapshot = default_snapshot;
+ snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
+ if (!snapdisk->snapshot)
+ snapdisk->snapshot = default_snapshot;
}
qsort(&snapdef->disks[0], snapdef->ndisks, sizeof(snapdef->disks[0]),
--
2.26.2
10:34 a.m.
New subject: [PATCH 07/13] virDomainSnapshotAlignDisks: Rename 'disk' -> 'snapdisk'
On a Wednesday in 2020, Peter Krempa wrote:
The 'disk' variable usually refers to a definition of a disk
from the
domain definition. Rename it to 'snapdisk' to be clear that we are
Sounds catchy. Have you bought the domain already?
talking about the snapshot disk definition especially since this
function also accesses the domain disk definition.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 54 ++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 08/13] virDomainSnapshotAlignDisks: Add 'domdef' local variable
There are multiple places accessing the domain definition. Extract it to
a local variable so that it's more clear what's happening.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index c835ec7333..a1cee01944 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -649,33 +649,34 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
int default_snapshot,
bool require_match)
{
+ virDomainDefPtr domdef = snapdef->parent.dom;
g_autoptr(virBitmap) map = NULL;
size_t i;
int ndisks;
- if (!snapdef->parent.dom) {
+ if (!domdef) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing domain in snapshot"));
return -1;
}
- if (snapdef->ndisks > snapdef->parent.dom->ndisks) {
+ if (snapdef->ndisks > domdef->ndisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("too many disk snapshot requests for domain"));
return -1;
}
/* Unlikely to have a guest without disks but technically possible. */
- if (!snapdef->parent.dom->ndisks)
+ if (!domdef->ndisks)
return 0;
- if (!(map = virBitmapNew(snapdef->parent.dom->ndisks)))
+ if (!(map = virBitmapNew(domdef->ndisks)))
return -1;
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i];
- int idx = virDomainDiskIndexByName(snapdef->parent.dom, snapdisk->name,
false);
+ int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
int disk_snapshot;
if (idx < 0) {
@@ -693,7 +694,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
ignore_value(virBitmapSetBit(map, idx));
snapdisk->idx = idx;
- disk_snapshot = snapdef->parent.dom->disks[idx]->snapshot;
+ disk_snapshot = domdef->disks[idx]->snapshot;
if (!snapdisk->snapshot) {
if (disk_snapshot &&
(!require_match ||
@@ -721,33 +722,33 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
snapdisk->src->path, snapdisk->name);
return -1;
}
- if (STRNEQ(snapdisk->name, snapdef->parent.dom->disks[idx]->dst)) {
+ if (STRNEQ(snapdisk->name, domdef->disks[idx]->dst)) {
VIR_FREE(snapdisk->name);
- snapdisk->name = g_strdup(snapdef->parent.dom->disks[idx]->dst);
+ snapdisk->name = g_strdup(domdef->disks[idx]->dst);
}
}
/* Provide defaults for all remaining disks. */
ndisks = snapdef->ndisks;
if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
- snapdef->parent.dom->ndisks - snapdef->ndisks) < 0)
+ domdef->ndisks - snapdef->ndisks) < 0)
return -1;
- for (i = 0; i < snapdef->parent.dom->ndisks; i++) {
+ for (i = 0; i < domdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk;
if (virBitmapIsBitSet(map, i))
continue;
snapdisk = &snapdef->disks[ndisks++];
snapdisk->src = virStorageSourceNew();
- snapdisk->name = g_strdup(snapdef->parent.dom->disks[i]->dst);
+ snapdisk->name = g_strdup(domdef->disks[i]->dst);
snapdisk->idx = i;
/* Don't snapshot empty drives */
- if (virStorageSourceIsEmpty(snapdef->parent.dom->disks[i]->src))
+ if (virStorageSourceIsEmpty(domdef->disks[i]->src))
snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
else
- snapdisk->snapshot = snapdef->parent.dom->disks[i]->snapshot;
+ snapdisk->snapshot = domdef->disks[i]->snapshot;
snapdisk->src->type = VIR_STORAGE_TYPE_FILE;
if (!snapdisk->snapshot)
--
2.26.2
10:34 a.m.
New subject: [PATCH 08/13] virDomainSnapshotAlignDisks: Add 'domdef' local variable
On a Wednesday in 2020, Peter Krempa wrote:
There are multiple places accessing the domain definition. Extract it
to
a local variable so that it's more clear what's happening.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 09/13] virDomainSnapshotAlignDisks: Extract domain disk definition to a local variable
Extract the disk def to a local variable so that it's more obvious
what's happening and it will also allow further simplification.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index a1cee01944..aeebe2fb33 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -677,6 +677,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i];
int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
+ virDomainDiskDefPtr domdisk = NULL;
int disk_snapshot;
if (idx < 0) {
@@ -685,6 +686,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
return -1;
}
+ domdisk = domdef->disks[idx];
+
if (virBitmapIsBitSet(map, idx)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
@@ -694,7 +697,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
ignore_value(virBitmapSetBit(map, idx));
snapdisk->idx = idx;
- disk_snapshot = domdef->disks[idx]->snapshot;
+ disk_snapshot = domdisk->snapshot;
if (!snapdisk->snapshot) {
if (disk_snapshot &&
(!require_match ||
@@ -722,9 +725,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
snapdisk->src->path, snapdisk->name);
return -1;
}
- if (STRNEQ(snapdisk->name, domdef->disks[idx]->dst)) {
+ if (STRNEQ(snapdisk->name, domdisk->dst)) {
VIR_FREE(snapdisk->name);
- snapdisk->name = g_strdup(domdef->disks[idx]->dst);
+ snapdisk->name = g_strdup(domdisk->dst);
}
}
--
2.26.2
10:35 a.m.
New subject: [PATCH 09/13] virDomainSnapshotAlignDisks: Extract domain disk definition to a local variable
On a Wednesday in 2020, Peter Krempa wrote:
Extract the disk def to a local variable so that it's more
obvious
what's happening and it will also allow further simplification.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 10/13] virDomainSnapshotAlignDisks: remove unnecessary 'tmp' variable
The converted string is used exactly once so we can call the conversion
without storing the result in a variable.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index aeebe2fb33..10eb584a1c 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -709,12 +709,10 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
snapdisk->snapshot != default_snapshot &&
!(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE
&&
disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
- const char *tmp;
-
- tmp = virDomainSnapshotLocationTypeToString(default_snapshot);
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode
'%s'"),
- snapdisk->name, tmp);
+ snapdisk->name,
+ virDomainSnapshotLocationTypeToString(default_snapshot));
return -1;
}
if (snapdisk->src->path &&
--
2.26.2
10:36 a.m.
New subject: [PATCH 10/13] virDomainSnapshotAlignDisks: remove unnecessary 'tmp' variable
On a Wednesday in 2020, Peter Krempa wrote:
The converted string is used exactly once so we can call the
conversion
without storing the result in a variable.
I presume this was done to reduce line length, not to reuse it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 11/13] virDomainSnapshotAlignDisks: clarify handing of snapshot location
Remove the use of the 'disk_snapshot' temporary variable since accessing
the disk definition now isn't that much longer to write and use expicit
value checks instead of the (non-)zero check to make it more obvious
what the code is doing.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 10eb584a1c..f6a827d2ff 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -678,7 +678,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i];
int idx = virDomainDiskIndexByName(domdef, snapdisk->name, false);
virDomainDiskDefPtr domdisk = NULL;
- int disk_snapshot;
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -697,24 +696,25 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
ignore_value(virBitmapSetBit(map, idx));
snapdisk->idx = idx;
- disk_snapshot = domdisk->snapshot;
- if (!snapdisk->snapshot) {
- if (disk_snapshot &&
+ if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) {
+ if (domdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT &&
(!require_match ||
- disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE))
- snapdisk->snapshot = disk_snapshot;
- else
+ domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
+ snapdisk->snapshot = domdisk->snapshot;
+ } else {
snapdisk->snapshot = default_snapshot;
+ }
} else if (require_match &&
snapdisk->snapshot != default_snapshot &&
!(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE
&&
- disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
+ domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' must use snapshot mode
'%s'"),
snapdisk->name,
virDomainSnapshotLocationTypeToString(default_snapshot));
return -1;
}
+
if (snapdisk->src->path &&
snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
--
2.26.2
10:33 a.m.
New subject: [PATCH 11/13] virDomainSnapshotAlignDisks: clarify handing of snapshot location
On 9/23/20 8:33 AM, Peter Krempa wrote:
Remove the use of the 'disk_snapshot' temporary variable
since accessing
the disk definition now isn't that much longer to write and use expicit
explicit
value checks instead of the (non-)zero check to make it more obvious
what the code is doing.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:38 a.m.
New subject: [PATCH 11/13] virDomainSnapshotAlignDisks: clarify handing of snapshot location
On a Wednesday in 2020, Peter Krempa wrote:
Remove the use of the 'disk_snapshot' temporary variable since
accessing
the disk definition now isn't that much longer to write and use expicit
explicit
value checks instead of the (non-)zero check to make it more obvious
what the code is doing.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 12/13] virDomainSnapshotAlignDisks: refactor extension to all disks
Last step of the algorithm in virDomainSnapshotAlignDisks is to extend
the array of disks to all VM's disk and provide defaults. This was done
by extending the array, adding defaults at the end and then sorting it.
This requires the 'idx' variable and also a separate sorting function.
If we store the pointer to existing snapshot disk definitions in a hash
table and create a new array of snapshot disk definitions, we can fill
the new array directly by either copying the definition from the old
array or adding the default.
This avoids the sorting step and thus even the need to store the index
of the domain disk altogether.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 48 +++++++++++++++-------------------------
1 file changed, 18 insertions(+), 30 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f6a827d2ff..7090e3a1b0 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -627,16 +627,6 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDefPtr def)
}
-static int
-virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
-{
- const virDomainSnapshotDiskDef *diska = a;
- const virDomainSnapshotDiskDef *diskb = b;
-
- /* Integer overflow shouldn't be a problem here. */
- return diska->idx - diskb->idx;
-}
-
/* Align def->disks to def->parent.dom. Sort the list of def->disks,
* filling in any missing disks or snapshot state defaults given by
* the domain, with a fallback to a passed in default. Convert paths
@@ -650,9 +640,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
bool require_match)
{
virDomainDefPtr domdef = snapdef->parent.dom;
- g_autoptr(virBitmap) map = NULL;
+ g_autoptr(virHashTable) map = virHashNew(NULL);
+ g_autofree virDomainSnapshotDiskDefPtr olddisks = NULL;
size_t i;
- int ndisks;
if (!domdef) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -670,9 +660,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
if (!domdef->ndisks)
return 0;
- if (!(map = virBitmapNew(domdef->ndisks)))
- return -1;
-
/* Double check requested disks. */
for (i = 0; i < snapdef->ndisks; i++) {
virDomainSnapshotDiskDefPtr snapdisk = &snapdef->disks[i];
@@ -687,14 +674,15 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
domdisk = domdef->disks[idx];
- if (virBitmapIsBitSet(map, idx)) {
+ if (virHashHasEntry(map, domdisk->dst)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' specified twice"),
snapdisk->name);
return -1;
}
- ignore_value(virBitmapSetBit(map, idx));
- snapdisk->idx = idx;
+
+ if (virHashAddEntry(map, domdisk->dst, snapdisk) < 0)
+ return -1;
if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) {
if (domdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT &&
@@ -729,21 +717,24 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
}
}
- /* Provide defaults for all remaining disks. */
- ndisks = snapdef->ndisks;
- if (VIR_EXPAND_N(snapdef->disks, snapdef->ndisks,
- domdef->ndisks - snapdef->ndisks) < 0)
- return -1;
+ olddisks = g_steal_pointer(&snapdef->disks);
+ snapdef->disks = g_new0(virDomainSnapshotDiskDef, domdef->ndisks);
+ snapdef->ndisks = domdef->ndisks;
for (i = 0; i < domdef->ndisks; i++) {
- virDomainSnapshotDiskDefPtr snapdisk;
+ virDomainDiskDefPtr domdisk = domdef->disks[i];
+ virDomainSnapshotDiskDefPtr snapdisk = snapdef->disks + i;
+ virDomainSnapshotDiskDefPtr existing;
- if (virBitmapIsBitSet(map, i))
+ /* copy existing disks */
+ if ((existing = virHashLookup(map, domdisk->dst))) {
+ memcpy(snapdisk, existing, sizeof(*snapdisk));
continue;
- snapdisk = &snapdef->disks[ndisks++];
+ }
+
+ /* Provide defaults for all remaining disks. */
snapdisk->src = virStorageSourceNew();
snapdisk->name = g_strdup(domdef->disks[i]->dst);
- snapdisk->idx = i;
/* Don't snapshot empty drives */
if (virStorageSourceIsEmpty(domdef->disks[i]->src))
@@ -756,9 +747,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapdef,
snapdisk->snapshot = default_snapshot;
}
- qsort(&snapdef->disks[0], snapdef->ndisks, sizeof(snapdef->disks[0]),
- virDomainSnapshotCompareDiskIndex);
-
/* Generate default external file names for external snapshot locations */
if (virDomainSnapshotDefAssignExternalNames(snapdef) < 0)
return -1;
--
2.26.2
10:35 a.m.
New subject: [PATCH 12/13] virDomainSnapshotAlignDisks: refactor extension to all disks
On 9/23/20 8:33 AM, Peter Krempa wrote:
Last step of the algorithm in virDomainSnapshotAlignDisks is to
extend
the array of disks to all VM's disk and provide defaults. This was done
by extending the array, adding defaults at the end and then sorting it.
This requires the 'idx' variable and also a separate sorting function.
If we store the pointer to existing snapshot disk definitions in a hash
table and create a new array of snapshot disk definitions, we can fill
the new array directly by either copying the definition from the old
array or adding the default.
This avoids the sorting step and thus even the need to store the index
of the domain disk altogether.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 48 +++++++++++++++-------------------------
1 file changed, 18 insertions(+), 30 deletions(-)
Nice cleanup.
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:43 a.m.
New subject: [PATCH 12/13] virDomainSnapshotAlignDisks: refactor extension to all disks
On a Wednesday in 2020, Peter Krempa wrote:
Last step of the algorithm in virDomainSnapshotAlignDisks is to
extend
the array of disks to all VM's disk and provide defaults. This was done
by extending the array, adding defaults at the end and then sorting it.
This requires the 'idx' variable and also a separate sorting function.
If we store the pointer to existing snapshot disk definitions in a hash
table and create a new array of snapshot disk definitions, we can fill
the new array directly by either copying the definition from the old
array or adding the default.
This avoids the sorting step and thus even the need to store the index
of the domain disk altogether.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.c | 48 +++++++++++++++-------------------------
1 file changed, 18 insertions(+), 30 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
8:33 a.m.
New subject: [PATCH 13/13] virDomainSnapshotDiskDef: Remove 'idx' field
It's no longer needed and is valid only after virDomainSnapshotAlignDisks
is called while holding the lock.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index b5b1ef2718..fbc9b17c54 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -63,7 +63,6 @@ typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr;
struct _virDomainSnapshotDiskDef {
char *name; /* name matching the <target dev='...' of the domain */
- int idx; /* index within snapshot->dom->disks that matches name */
int snapshot; /* virDomainSnapshotLocation */
/* details of wrapper external file. src is always non-NULL.
--
2.26.2
10:43 a.m.
New subject: [PATCH 13/13] virDomainSnapshotDiskDef: Remove 'idx' field
On a Wednesday in 2020, Peter Krempa wrote:
It's no longer needed and is valid only after
virDomainSnapshotAlignDisks
is called while holding the lock.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/snapshot_conf.h | 1 -
1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1521
days inactive
1521
days old
28 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa