This commit adds the option for setting per-disk snapshot name for
RBD disks. All other disk types are still disallowed to use the
snapshotName attribute.

Signed-off-by: Or Ozeri <oro@il.ibm.com>
---
 src/qemu/qemu_snapshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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"),
-- 
2.25.1

On Wed, Feb 15, 2023 at 05:28:21 -0600, Or Ozeri wrote:
> This commit adds the option for setting per-disk snapshot name for
> RBD disks. All other disk types are still disallowed to use the
> snapshotName attribute.
> 
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
>  src/qemu/qemu_snapshot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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.

> -----Original Message-----
> From: Peter Krempa <pkrempa@redhat.com>
> Sent: Monday, 20 February 2023 16:13
> To: Or Ozeri <ORO@il.ibm.com>
> Cc: libvir-list@redhat.com; Danny Harnik <DANNYH@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?