docs: Add notes for VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA

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

Posted by Han Han 5 years, 5 months ago

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 <hhan@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
-- 
2.19.1

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

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

Posted by Peter Krempa 5 years, 5 months ago

On Wed, Nov 14, 2018 at 15:19:20 +0800, Han Han wrote:
> 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 <hhan@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.
> + * */

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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Andrea Bolognani 5 years, 5 months ago

On Wed, 2018-11-14 at 15:19 +0800, Han Han wrote:
> 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.

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.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

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

Posted by Han Han 5 years, 5 months ago

On Thu, Nov 15, 2018 at 12:36 AM Andrea Bolognani <abologna@redhat.com>
wrote:

> On Wed, 2018-11-14 at 15:19 +0800, Han Han wrote:
> > 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.
>
> 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
>
>

-- 
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.

Email: hhan@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list