[libvirt] [PATCH 04/13] backup: Parse and output checkpoint XML

[Peter Krempa]

On Tue, Jun 18, 2019 at 22:47:45 -0500, Eric Blake wrote:
> Add a new file checkpoint_conf.c that performs the translation to and
> from new XML describing a checkpoint. The code shares a common base
> class with snapshots, since a checkpoint similarly represents the
> domain state at a moment in time. Add some basic testing of round trip
> XML handling through the new code.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/checkpoint_conf.h                    |  94 +++
>  src/conf/virconftypes.h                       |   3 +
>  po/POTFILES                                   |   1 +
>  src/conf/Makefile.inc.am                      |   2 +
>  src/conf/checkpoint_conf.c                    | 575 ++++++++++++++++++
>  src/libvirt_private.syms                      |   8 +
>  tests/Makefile.am                             |   9 +-
>  .../internal-active-invalid.xml               |  53 ++
>  .../internal-inactive-invalid.xml             |  53 ++
>  tests/domaincheckpointxml2xmltest.c           | 223 +++++++
>  10 files changed, 1019 insertions(+), 2 deletions(-)
>  create mode 100644 src/conf/checkpoint_conf.h
>  create mode 100644 src/conf/checkpoint_conf.c
>  create mode 100644 tests/domaincheckpointxml2xmlout/internal-active-invalid.xml
>  create mode 100644 tests/domaincheckpointxml2xmlout/internal-inactive-invalid.xml
>  create mode 100644 tests/domaincheckpointxml2xmltest.c



> diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
> new file mode 100644
> index 0000000000..a7a34d3887
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.c
> @@ -0,0 +1,575 @@

[...]

> +static int
> +virDomainCheckpointDiskDefParseXML(xmlNodePtr node,
> +                                   xmlXPathContextPtr ctxt,
> +                                   virDomainCheckpointDiskDefPtr def)
> +{
> +    int ret = -1;
> +    char *checkpoint = NULL;
> +    char *bitmap = NULL;
> +    xmlNodePtr saved = ctxt->node;
> +
> +    ctxt->node = node;
> +
> +    def->name = virXMLPropString(node, "name");
> +    if (!def->name) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("missing name from disk checkpoint element"));

Shouldn't schema validation catch this? If schema validation is not
enabled note that there isn't a second chance to do it.

> +        goto cleanup;
> +    }

[...]

> +
> +    if (!(def = virDomainCheckpointDefNew()))
> +        return NULL;
> +
> +    def->parent.name = virXPathString("string(./name)", ctxt);
> +    if (def->parent.name == NULL) {
> +        if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("a redefined checkpoint must have a name"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    def->parent.description = virXPathString("string(./description)", ctxt);
> +
> +    if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> +        if (virXPathLongLong("string(./creationTime)", ctxt,
> +                             &def->parent.creationTime) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("missing creationTime from existing checkpoint"));

While I can see the point that we do the same thing in the snapshot XML
parser, having validation intermixed with the parser is not great for
the future.

> +            goto cleanup;
> +        }
> +
> +        def->parent.parent_name = virXPathString("string(./parent/name)", ctxt);
> +
> +        if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
> +            int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                              VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
> +            xmlNodePtr domainNode = virXPathNode("./domain", ctxt);

ACK, this adds bunch of technical debt based on the fact that it's
mostly copied form snapshots, but I trust you will address that later.

I'd strongly prefer if we'd do schema validation unconditionally for the
new APIs though since we won't get to fix that one later.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190619/34cd1b0f/attachment-0001.sig>