On Tue, Aug 23, 2022 at 18:32:25 +0200, Pavel Hrdina wrote:
> With external snapshots we need to modify the metadata bit more then
> what is required for internal snapshots. Mainly the storage source
> location changes with every external snapshot.
>
> This means that if we delete non-leaf snapshot we need to update all
> children snapshots and modify the disk sources for all affected disks.
>
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
> src/qemu/qemu_snapshot.c | 116 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 116 insertions(+)
>
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 64ee395230..37ae3f04d0 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2380,6 +2380,109 @@ qemuSnapshotChildrenReparent(void *payload,
> }
>
>
> +typedef struct _qemuSnapshotUpdateDisksData qemuSnapshotUpdateDisksData;
> +struct _qemuSnapshotUpdateDisksData {
> + virDomainMomentObj *snap;
> + virDomainObj *vm;
> + virQEMUDriver *driver;
'driver' is present in private data of 'vm'
> + int error;
> + int (*writeMetadata)(virDomainObj *, virDomainMomentObj *,
> + virDomainXMLOption *, const char *);
The only function pointer that is ever populated here is
qemuDomainSnapshotWriteMetadata. So you can use that directly from the
function that uses this.
> +};
> +
> +
> +static int
> +qemuSnapshotUpdateDisksSingle(virDomainMomentObj *snap,
> + virDomainDef *def,
> + virDomainDef *parentDef,
> + virDomainSnapshotDiskDef *snapDisk)
> +{
> + virDomainDiskDef *disk = NULL;
> +
> + if (!(disk = qemuDomainDiskByName(def, snapDisk->name)))
> + return -1;
> +
> + if (virDomainSnapshotIsExternal(snap)) {
So IIUC, this bit is done only for external snapshots ...
> + virDomainDiskDef *parentDisk = NULL;
> +
> + if (!(parentDisk = qemuDomainDiskByName(parentDef, snapDisk->name)))
> + return -1;
> +
> + if (virStorageSourceIsSameLocation(snapDisk->src, disk->src)) {
> + virObjectUnref(disk->src);
> + disk->src = virStorageSourceCopy(parentDisk->src, false);
> + }
> + }
> +
> + if (disk->src->backingStore) {
... but this even for internal ones? That doesn't seem right.
> + virStorageSource *cur = disk->src;
> + virStorageSource *next = disk->src->backingStore;
> +
> + while (next) {
> + if (virStorageSourceIsSameLocation(snapDisk->src, next)) {
> + cur->backingStore = next->backingStore;
> + next->backingStore = NULL;
> + virObjectUnref(next);
> + break;
> + }
So you are looking up the image corresponding to the snapshot disk and
trying to move it one level up, right?
> +
> + cur = next;
> + next = cur->backingStore;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuSnapshotDeleteUpdateDisks(void *payload,
> + const char *name G_GNUC_UNUSED,
> + void *opaque)
> +{
> + virDomainMomentObj *snap = payload;
> + qemuSnapshotUpdateDisksData *data = opaque;
> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(data->driver);
> + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(data->snap);
> + ssize_t i;
> +
> + if (data->error < 0)
> + return 0;
At this point I'm afraid we can't afford to deal with errors the usual
way but have to try to update as much metadata as possible.
If the first error breaks up everything then the state of the disk
images will not correspond to the metadata.
> +
> + for (i = 0; i < snapdef->ndisks; i++) {
> + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
> +
> + if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO)
> + continue;
> +
> + if (qemuSnapshotUpdateDisksSingle(snap, snap->def->dom,
> + data->snap->def->dom, snapDisk) < 0) {
> + data->error = -1;
> + return 0;
> + }
> +
> + if (snap->def->inactiveDom) {
> + virDomainDef *dom = data->snap->def->inactiveDom;
> +
> + if (!dom)
> + dom = data->snap->def->dom;
> +
> + if (qemuSnapshotUpdateDisksSingle(snap, snap->def->inactiveDom,
> + dom, snapDisk) < 0) {
The failure of qemuDomainDiskByName, in this case probably should not be
fatal, if the next-boot config diverges from the running config.
> + data->error = -1;
> + return 0;
> + }
> + }
> + }
> +
> + data->error = data->writeMetadata(data->vm,
> + snap,
> + data->driver->xmlopt,
> + cfg->snapshotDir);
> + return 0;
> +}
