[libvirt] [PATCH] docs: Add notes for VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA

[Han Han]

When listing snapshot with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, it
always returns 0 or no snapshot. Because we never implement funtions
to list no-metadata snapshot in virDomainSnapshotObjListGetNames():

if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) ==
VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA)
return 0;

Add notes for that flag.

Please update the comment and man page of that flag when no-metadata
snapshot list is implemented in the future.

Signed-off-by: Han Han <***@redhat.com>
---
include/libvirt/libvirt-domain-snapshot.h | 5 ++++-
tools/virsh.pod | 3 +++
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index 20771f9b1e..2e19a52a5c 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -93,7 +93,10 @@ char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
* of flag (1<<0) depends on which function it is passed to; but serves
* to toggle the per-call default of whether the listing is shallow or
* recursive. Remaining bits come in groups; if all bits from a group are
- * 0, then that group is not used to filter results. */
+ * 0, then that group is not used to filter results. Internal functions
+ * for listing no-metadata snapshots aren't implemented. Functions above
+ * will return 0 when VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA is used.
+ * */
typedef enum {
VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots
with no parents, when
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 86c041d575..b3d3840c2b 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4689,6 +4689,9 @@ B<undefine> of a persistent domain, or be lost on B<destroy> of
a transient domain. Likewise, if I<--no-metadata> is specified,
the list will be filtered to just snapshots that exist without
the need for libvirt metadata.
+Note that - It will return no snapshot when I<--no-metadata> is
+used since internal functions for listing no-metadata snapshot
+are not implemented.

If I<--inactive> is specified, the list will be filtered to snapshots
that were taken when the domain was shut off. If I<--active> is

[Andrea Bolognani]

Post by Han Han
When listing snapshot with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, it
always returns 0 or no snapshot. Because we never implement funtions
if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) ==
VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA)
return 0;
Add notes for that flag.
Please update the comment and man page of that flag when no-metadata
snapshot list is implemented in the future.

I could be missing some information, but from a quick look at the
commit message and the patch it looks to me like you're documenting
a known limitation instead of, you know, addressing it :)

If you are able to fix the issue yourself, then please do so;
otherwise, filing a bug seems like it would be a more appropriate
course of action.

[Han Han]

Post by Andrea Bolognani

Post by Han Han
When listing snapshot with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, it
always returns 0 or no snapshot. Because we never implement funtions
if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) ==
VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA)
return 0;
Add notes for that flag.
Please update the comment and man page of that flag when no-metadata
snapshot list is implemented in the future.

I could be missing some information, but from a quick look at the
commit message and the patch it looks to me like you're documenting
a known limitation instead of, you know, addressing it :)
Bug filed as : https://bugzilla.redhat.com/show_bug.cgi?id=1650419
If you are able to fix the issue yourself, then please do so;
otherwise, filing a bug seems like it would be a more appropriate
course of action.
--
Andrea Bolognani / Red Hat / Virtualization

[Peter Krempa]

Post by Han Han
When listing snapshot with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, it
always returns 0 or no snapshot. Because we never implement funtions
if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) ==
VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA)
return 0;
Add notes for that flag.
Please update the comment and man page of that flag when no-metadata
snapshot list is implemented in the future.
---
include/libvirt/libvirt-domain-snapshot.h | 5 ++++-
tools/virsh.pod | 3 +++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index 20771f9b1e..2e19a52a5c 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -93,7 +93,10 @@ char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
* of flag (1<<0) depends on which function it is passed to; but serves
* to toggle the per-call default of whether the listing is shallow or
* recursive. Remaining bits come in groups; if all bits from a group are
- * 0, then that group is not used to filter results. */
+ * 0, then that group is not used to filter results. Internal functions
+ * for listing no-metadata snapshots aren't implemented. Functions above
+ * will return 0 when VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA is used.
+ * */

This really is hypervisor dependent. E.g. virtualbox driver does not
even have a concept of no-metadata snapshots as they are always tracked
with virtualbox. In this case the comment is valid only for qemu driver
since:

1) it supports (internal) snapshots
2) libvirt is required to have additional data, since qemu itself can't
trac everything internally
3) it's not implemented.

On the other hand. I don't ever expect us to implement support for
no-metadata snapshots as it's a very narrow corner case which was
created by the users either externally or knowingly and internally poses
a lot of challenges (e.g. we won't have the XML definition from the
snapshot-time and thus can't verify ABI stability).

Additionally some hypervisor drivers don't even support snapshots at
all.

I'd go with a message along "Some hypervisors may not support
metadata-less snapshots." or similar.

Note that we can't just reject the flag for the qemu implementation as
it might break some users.