> -----Original Message----- > From: Peter Krempa <pkrempa@xxxxxxxxxx> > Sent: Monday, 20 February 2023 16:00 > To: Or Ozeri <ORO@xxxxxxxxxx> > Cc: libvir-list@xxxxxxxxxx; Danny Harnik <DANNYH@xxxxxxxxxx> > Subject: [EXTERNAL] Re: [PATCH v2 3/6] qemu: Add internal support for > active disk internal snapshots > > This modification is done to a function named > `qemuSnapshotCreateActiveExternal` but clearly enabled creation of internal > snapshots. You'll need to address that one too. By address, you mean rename? It treats both external and disks-only (which can be internal / external / mixed) So maybe qemuSnapshotCreateActiveExternalOrDisksOnly? I can't think of a neat name for it :\ > > Since your patches add intermingling (at least) partial of snapshots the code > will need to be split up separately. > > 1) check that the old-style full internal snapshot is requested and deal with > that as a special case This check already exists, so I guess no change is required? > 2) separate qemuSnapshotCreateActiveExternal into: > - 2.1) - the bit that creates the external memory snapshot > - 2.2) - the bit that creates the external disk snapshot You mean separate each of the bits mentioned above to a new function? The bit that creates the memory snapshot also pauses the VM, and unpause after all is done (including the disks snapshot). I'm guessing you do not want to include this as part of the 2.1 mentioned above? > - 2.3) - modify the bit that creates external disk snapshot to also > do the internal disk snapshot for rbd > - 2.4) - call them in proper sequence. I suspect you also want to do > a external memory snapshot including the internal RBD > snapshot as s transaction. Actually no, there is no current use-case for that (that I know of). > > Good bit is that the oldschool internal snapshot is a completely separate case > and can't be intermingled here. > > But the existing code must be modified to honour the change of semantics > you are introducing.
