On 10/3/24 15:46, Peter Krempa wrote:
As explained in the commit which added the new internal snapshot deletion code we don't want to do any form of strict checking whether the libvirt metadata is consistent with the on-disk state as we didn't historically do that. In order to be able to spot the cases add a warning into the logs if such state is encountered. While warnings are easy to miss it's the only reasonable way to do that. Users will be encouraged to file an issue with the information, without requiring them to enable debug logs as the reproduction of that issue may include very old historical state. The checker is deliberately added separately so that it can be easily reverted once it's no longer needed. Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx> --- src/qemu/qemu_snapshot.c | 54 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 4927ca0bfc..02c876c881 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3731,6 +3731,14 @@ qemuSnapshotActiveInternalDeleteGetDevices(virDomainObj *vm, g_auto(GStrv) devices = g_new0(char *, vm->def->ndisks + 2); size_t ndevs = 0; size_t i = 0; + /* variables below are used for checking of corner cases */ + g_autoptr(GHashTable) foundDisks = virHashNew(NULL); + g_auto(virBuffer) errMissingSnap = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) errUnexpectedSnap = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) errExtraDisks = VIR_BUFFER_INITIALIZER; + GHashTableIter htitr; + void *key; + bool warn = false; if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, VIR_ASYNC_JOB_SNAPSHOT))) return NULL; @@ -3759,6 +3767,7 @@ qemuSnapshotActiveInternalDeleteGetDevices(virDomainObj *vm, continue; devices[ndevs++] = g_strdup(format_nodename); + g_hash_table_insert(foundDisks, g_strdup(domdisk->dst), NULL); } if (vm->def->os.loader && @@ -3775,6 +3784,51 @@ qemuSnapshotActiveInternalDeleteGetDevices(virDomainObj *vm, } } + /* We currently don't want this code to become stricter than what 'delvm' + * did thus we'll report if the on-disk state mismatches the snapshot + * definition only as a warning */ + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = snapdef->disks + i; + + switch (snapdisk->snapshot) { + case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: + if (g_hash_table_contains(foundDisks, snapdisk->name)) { + g_hash_table_remove(foundDisks, snapdisk->name); + } else { + virBufferAsprintf(&errMissingSnap, "%s ", snapdisk->name); + warn = true; + } + break; + + case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_MANUAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_NO: + case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: + case VIR_DOMAIN_SNAPSHOT_LOCATION_LAST: + if (g_hash_table_contains(foundDisks, snapdisk->name)) { + virBufferAsprintf(&errUnexpectedSnap, "%s ", snapdisk->name); + warn = true; + } else { + g_hash_table_remove(foundDisks, snapdisk->name); + }
This if-else statement looks illogical or I don't understand how
it works. Probably you meant
if (!g_hash_table_contains... ?
+ }
+ }
+
+ g_hash_table_iter_init(&htitr, foundDisks);
+
+ while (g_hash_table_iter_next(&htitr, &key, NULL)) {
+ warn = true;
+ virBufferAsprintf(&errExtraDisks, "%s ", (const char *) key);
+ }
+
+ if (warn) {
+ VIR_WARN("inconsistent internal snapshot state (deletion): VM='%s' snapshot='%s' missing='%s' unexpected='%s' extra='%s",
+ vm->def->name, snapname,
+ virBufferCurrentContent(&errMissingSnap),
+ virBufferCurrentContent(&errUnexpectedSnap),
+ virBufferCurrentContent(&errExtraDisks));
+ }
+
return g_steal_pointer(&devices);
}
