[libvirt] [PATCHv2 2/4] snapshot: add virsh back-compat support for new filters

[Peter Krempa]

On 11/15/12 00:36, Eric Blake wrote:
> Snapshot filtering based on types is useful enough to add
> back-compat support into virsh.  It is also rather easy - all
> versions of libvirt that don't understand the new filter flags
> already gave us sufficient information in a single XML field
> to reconstruct all the information we need (that is, it isn't
> until libvirt 1.0.1 that we have more interesting types of
> snapshots, such as offline external).
>
> * tools/virsh-snapshot.c (vshSnapshotFilter): New function.
> (vshSnapshotListCollect): Add fallback support.
> ---
>   tools/virsh-snapshot.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index 0363e19..7cd2966 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -39,6 +39,7 @@
>   #include "util.h"
>   #include "virsh-domain.h"
>   #include "xml.h"
> +#include "conf/snapshot_conf.h"
>
>   /* Helper for snapshot-create and snapshot-create-as */
>   static bool
> @@ -712,6 +713,67 @@ cleanup:
>       return ret;
>   }
>
> +/* Helper function to filter snapshots according to status and
> + * location portion of flags.  Returns 0 if filter excluded snapshot
> + * (or if snapshot is already NULL), 1 if snapshot is okay, and -1 on
> + * failure with error reported.  */
> +static int
> +vshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot,
> +                  unsigned int flags)
> +{
> +    char *xml = NULL;
> +    xmlDocPtr xmldoc = NULL;
> +    xmlXPathContextPtr ctxt = NULL;
> +    int ret = -1;
> +    char *state = NULL;
> +
> +    if (!snapshot)
> +        return 0;
> +
> +    xml = virDomainSnapshotGetXMLDesc(snapshot, 0);
> +    if (!xml)
> +        goto cleanup;
> +
> +    xmldoc = virXMLParseStringCtxt(xml, _("(domain_snapshot)"), &ctxt);
> +    if (!xmldoc)
> +        goto cleanup;
> +
> +    /* Libvirt 1.0.1 and newer never call this function, because the
> +     * filtering is already supported by the listing functions.  Older
> +     * libvirt lacked /domainsnapshot/memory, but was also limited in
> +     * the types of snapshots it could create: if state was disk-only,
> +     * the snapshot is external; all other snapshots are internal.  */
> +    state = virXPathString("string(/domainsnapshot/state)", ctxt);
> +    if (!state)
> +        goto cleanup;
> +    if (STREQ(state, "disk-snapshot")) {
> +        ret = ((flags & (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
> +                         VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)) ==
> +               (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY |
> +                VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL));
> +    } else {
> +        if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL))
> +            ret = 0;
> +        else if (STREQ(state, "shutoff"))
> +            ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE);
> +        else
> +            ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE);
> +    }
> +
> +cleanup:
> +    if (ret < 0) {
> +        vshReportError(ctl);
> +        vshError(ctl, "%s", _("unable to perform snapshot filtering"));

Hm, reporting of libvirt's error isn't really needed here. When the 
filtering fails everything should fail gracefully and the error would be 
printed by vshCmdRun at the end when the command fails. The only benefit 
of this is that the error from libvirt is printed before the more 
specific error. In other commands we just print a local error and then 
let the libvirt error to be printed by the command handler.

> +    } else {
> +        vshResetLibvirtError();
> +    }
> +    VIR_FREE(state);
> +    xmlXPathFreeContext(ctxt);
> +    xmlFreeDoc(xmldoc);
> +    VIR_FREE(xml);
> +    return ret;
> +}
> +

ACK when you remove the error message printing or provide a explanation 
why you chose that approach.

Peter