On Tue, Jul 16, 2024 at 10:32:56AM +0200, Peter Krempa wrote:
> On Tue, Jul 16, 2024 at 10:21:12 +0200, Denis V. Lunev via Devel wrote:
> > On 7/16/24 00:42, Nikolai Barybin wrote:
> > > The usage of HMP commands are highly discouraged by qemu. Moreover,
> > > current snapshot creation routine does not provide flexibility in
> > > choosing target device for VM state snapshot.
> > >
> > > This patch makes use of QMP commands snapshot-save/delete and by
> > > default chooses first writable disk (if present) as target for VM
> > > state, NVRAM - otherwise.
> > >
> > > Signed-off-by: Nikolai Barybin <nikolai.barybin@xxxxxxxxxxxxx>
> > > ---
> > > src/qemu/qemu_snapshot.c | 158 ++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 148 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > > index f5260c4a22..83949a9a27 100644
> > > --- a/src/qemu/qemu_snapshot.c
> > > +++ b/src/qemu/qemu_snapshot.c
> > > @@ -308,6 +308,96 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
> > > return ret;
> > > }
> > > +static int
> > > +qemuSnapshotActiveInternalGetWrdevListHelper(virDomainObj *vm,
> > > + char **wrdevs)
> > > +{
> > > + size_t wrdevCount = 0;
> > > + size_t i = 0;
> > > +
> > > + for (i = 0; i < vm->def->ndisks; i++) {
> > > + virDomainDiskDef *disk = vm->def->disks[i];
> > > + if (!disk->src->readonly) {
> > > + wrdevs[wrdevCount] = g_strdup(disk->src->nodenameformat);
> > > + wrdevCount++;
> > > + }
> > > + }
> > > +
> > > + if (wrdevCount == 0) {
> > > + if (vm->def->os.loader->nvram) {
> > > + wrdevs[0] = g_strdup(vm->def->os.loader->nvram->nodenameformat);
> > > + } else {
> > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > + _("no writable device for internal snapshot creation/deletion"));
> > > + return -1;
> > > + }
> > > + }
> > > +
> >
> > should we select NVRAM at all?
> >
> > IMHO that would be very problematic.
> > I think that we should generate error in this case.
>
> No NVRAM image must *never* be selected as target for the VM state.
> Libvirt curretnly forbids internal snapshots with UEFI precisely because
> NVRAM image could have been selected historically and we do not want
> that ever happening.
If we DO have a writable disk though, and the NVRAM is in qcow2 format,
then we should be including NVRAM in the snapshot operation, but the
state snapshot should be stored in the primary disk, not the NVRAM.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
