Re: [PATCH v4 3/8] snapshot: Tweaks to support new bulk dumpxml/import API

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Mon, Mar 11, 2019 at 09:38:34PM -0500, Eric Blake wrote:
> Change the return value of virDomainSnapshotObjLisParse() to return
> the number of snapshots imported, and allow a return of 0 (the
> original proposal of adding a flag to virDomainSnapshotCreateXML
> required returning an arbitrary non-NULL snapshot, but with a new API
> that returns a count, we are no longer constrained to a non-empty
> list).
>
> Change virDomainSnapshotObjListFormat()'s flags argument to be
> the new public virDomainGetSnapshotsXMLFlags, since it is easy
> to support both flag values.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/conf/snapshot_conf.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 8235d7c526..3f24a80f76 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -507,8 +507,10 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def,
> }
>
>
> -/* Parse a <snapshots> XML entry into snapshots, which must start empty.
> - * Any <domain> sub-elements of a <domainsnapshot> must match domain_uuid.
> +/* Parse a <snapshots> XML entry into snapshots, which must start
> + * empty.  Any <domain> sub-elements of a <domainsnapshot> must match
> + * domain_uuid.  @flags is virDomainSnapshotParseFlags.  Return the

Do we need to pass (and check) the flags here?
Given this series, it would also make sense to me to drop the flags
argument and just imply the ones that are needed for bulk parse.

> + * number of snapshots parsed, or -1 on error.
>  */
> int
> virDomainSnapshotObjListParse(const char *xmlStr,
> @@ -562,11 +564,6 @@ virDomainSnapshotObjListParse(const char *xmlStr,
>
>     if ((n = virXPathNodeSet("./domainsnapshot", ctxt, &nodes)) < 0)
>         goto cleanup;
> -    if (!n) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("expected at least one <domainsnapshot> child"));
> -        goto cleanup;
> -    }
>
>     for (i = 0; i < n; i++) {
>         virDomainSnapshotDefPtr def;
> @@ -601,7 +598,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
>         (*current_snap)->def->current = true;
>     }
>
> -    ret = 0;
> +    ret = n;
>  cleanup:
>     if (ret < 0) {
>         /* There were no snapshots before this call; so on error, just
> @@ -1025,8 +1022,9 @@ virDomainSnapshotFormatOne(void *payload,
> }
>
>
> -/* Format the XML for all snapshots in the list into buf. On error,
> - * clear the buffer and return -1. */
> +/* Format the XML for all snapshots in the list into buf. @flags is
> + * virDomainGetSnapshotsXMLFlags. On error, clear the buffer and
> + * return -1. */
> int
> virDomainSnapshotObjListFormat(virBufferPtr buf,
>                                const char *uuidstr,
> @@ -1041,17 +1039,23 @@ virDomainSnapshotObjListFormat(virBufferPtr buf,
>         .uuidstr = uuidstr,
>         .caps = caps,
>         .xmlopt = xmlopt,
> -        .flags = flags,
> +        .flags = 0,
>     };

You can use virXMLFormatElement here:

   VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
   VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER;

   data.buf = &childBuf;


> +    bool topological = flags & VIR_DOMAIN_GET_SNAPSHOTS_XML_TOPOLOGICAL;
>
> +    virCheckFlags(VIR_DOMAIN_GET_SNAPSHOTS_XML_SECURE |
> +                  VIR_DOMAIN_GET_SNAPSHOTS_XML_TOPOLOGICAL, -1);
> +
> +    if (flags & VIR_DOMAIN_GET_SNAPSHOTS_XML_SECURE)
> +        data.flags |= VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE;

This:
>     virBufferAddLit(buf, "<snapshots");
>     if (current_snapshot)
>         virBufferEscapeString(buf, " current='%s'",
>                               current_snapshot->def->name);
>     virBufferAddLit(buf, ">\n");
>     virBufferAdjustIndent(buf, 2);
> -    if (virDomainSnapshotForEach(snapshots, false, virDomainSnapshotFormatOne,
> -                                 &data) < 0) {
> +    if (virDomainSnapshotForEach(snapshots, topological,
> +                                 virDomainSnapshotFormatOne, &data) < 0) {

would become:

   if (current_snapshot) {
       virBufferEscapeString(&attrBuf, " current='%s'",
                             current_snapshot->def->name);
   }
   virBufferSetChildIndent(&childBuf, buf);

   if (virDomainSnapshotForEach() < 0)
      ...

   if (virXMLFormatElement(buf, "snapshots", &attrBuf, &childBuf) < 0) {
       virBufferFreeAndReset(buf); /* as required by the function
                                      documentation */
       return -1;
   }

With the benefit of outputting a non-pair tag for empty snapshot list.

>         virBufferFreeAndReset(buf);
>         return -1;
>     }

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature

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