[PATCH v2 5/6] qemu: Allow setting per-disk snapshot name for RBD disks

[Or Ozeri]

> -----Original Message-----
> From: Peter Krempa <pkrempa at redhat.com>
> Sent: Monday, 20 February 2023 16:13
> To: Or Ozeri <ORO at il.ibm.com>
> Cc: libvir-list at redhat.com; Danny Harnik <DANNYH at il.ibm.com>
> Subject: [EXTERNAL] Re: [PATCH v2 5/6] qemu: Allow setting per-disk
> snapshot name for RBD disks
> 
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index
> > a10bdf7bf2..c72bdb4723 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -664,7 +664,7 @@ qemuSnapshotPrepare(virDomainObj *vm,
> >                  return -1;
> >              }
> >
> > -            if (disk->snapshot_name) {
> > +            if (disk->snapshot_name && !is_raw_rbd) {
> >                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >                                 _("snapshot name setting for disk %s unsupported "
> >                                   "for storage type %s"),
> 
> I don't see anything that updates the equivalent of dom_disk->src-
> >snapshot after the snapshot:
> 
> The 'snapshot' property is filled as the equivalent property when formatting
> the backend definition for the 'rbd' disk.
> 
> In case when the 'snapshot' field is meant to actually mean label the 'old'
> state. You then must actually tweak the snapshot metadata to point to it.
> That will allow proper reversion of the image.

We actually block reverting to this type of snapshot, see
" revert to a non-full internal snapshot not supported yet" in qemuSnapshotRevertValidate.
Given this, is this change still required?
Actually I'm not sure I understand. Is setting dom_disk->src->snapshot enough?
Or can you please point me to another place in the code where a change is required?