The docs talked about an active snapshot when they meant an active
domain; they also claimed the flag was a no-op for hypervisors with no
snapshot metadata even though the flag is rejected as unrecognized for
hypervisors with no snapshot support at all.

Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/libvirt-domain.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index e2594a3392..3d12e7c125 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -6270,10 +6270,11 @@ virDomainUndefine(virDomainPtr domain)
  * virDomainSnapshotNum()), then including
  * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA in @flags will also remove
  * that metadata.  Omitting the flag will cause the undefine of an
- * inactive domain to fail.  Active snapshots will retain snapshot
- * metadata until the (now-transient) domain halts, regardless of
- * whether this flag is present.  On hypervisors where snapshots do
- * not use libvirt metadata, this flag has no effect.
+ * inactive domain with snapshots to fail.  Active domains will retain
+ * snapshot metadata until the (now-transient) domain halts,
+ * regardless of whether this flag is present.  On hypervisors that
+ * support snapshots, but where snapshots do not use libvirt metadata,
+ * this flag has no effect.
  *
  * If the domain has any nvram specified, the undefine process will fail
  * unless VIR_DOMAIN_UNDEFINE_KEEP_NVRAM is specified, or if
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Jul 05, 2019 at 23:37:29 -0500, Eric Blake wrote:
> The docs talked about an active snapshot when they meant an active
> domain; they also claimed the flag was a no-op for hypervisors with no
> snapshot metadata even though the flag is rejected as unrecognized for
> hypervisors with no snapshot support at all.
> 
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/libvirt-domain.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

I'd probably go for the trivial addition of the flag to all of the
undefine APIs since that does not require clients from encoding the
knowledge whether the given hypervisor supports snapshots at all.

This works too though as we'd reject the flag at this point anyways.

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 7/8/19 2:40 AM, Peter Krempa wrote:
> On Fri, Jul 05, 2019 at 23:37:29 -0500, Eric Blake wrote:
>> The docs talked about an active snapshot when they meant an active
>> domain; they also claimed the flag was a no-op for hypervisors with no
>> snapshot metadata even though the flag is rejected as unrecognized for
>> hypervisors with no snapshot support at all.
>>
>> Reported-by: Peter Krempa <pkrempa@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  src/libvirt-domain.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> I'd probably go for the trivial addition of the flag to all of the
> undefine APIs since that does not require clients from encoding the
> knowledge whether the given hypervisor supports snapshots at all.

I can do that as well.  But as you observed, if you have a new virsh
talking to an old libvirtd, it doesn't help, and I don't see a problem
with the current documentation wording even if other drivers (silently)
accept and ignore the flag.

> 
> This works too though as we'd reject the flag at this point anyways.
> 
> ACK

I'll post a followup mail for review on adding no-op support in the
remaining domains.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list