[libvirt] [PATCHv1.5 16/27] snapshot: conf: Use common parsing and formatting functions for source

[Michal Privoznik]

On 26.11.2013 17:48, Peter Krempa wrote:
> Disk source elements for snapshots were using separate code from our
> config parser. As snapshots can be stored on more than just regular
> files, we will need the universal parser to allow us to expose a variety
> of snapshot disk targets. This patch reuses the config parsers and
> formatters to do the job.
> 
> This initial support only changes the code without any visible XML
> change.
> ---
>  src/conf/snapshot_conf.c | 70 +++++++++++++++++++++++++++++++++---------------
>  src/conf/snapshot_conf.h |  1 +
>  2 files changed, 49 insertions(+), 22 deletions(-)
> 
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 94a74d2..7258daa 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -128,27 +128,42 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
>          }
>      }
> 
> -    cur = node->children;
> -    while (cur) {
> -        if (cur->type == XML_ELEMENT_NODE) {
> -            if (!def->file &&
> -                xmlStrEqual(cur->name, BAD_CAST "source")) {
> -                def->file = virXMLPropString(cur, "file");
> -            } else if (!def->format &&
> -                       xmlStrEqual(cur->name, BAD_CAST "driver")) {
> -                char *driver = virXMLPropString(cur, "type");
> -                def->format = virStorageFileFormatTypeFromString(driver);
> -                if (def->format <= 0) {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                   _("unknown disk snapshot driver '%s'"),
> -                                   driver);
> -                    VIR_FREE(driver);
> -                    goto cleanup;
> -                }
> +    def->type = -1;
> +
> +    for (cur = node->children; cur; cur = cur->next) {
> +        if (cur->type != XML_ELEMENT_NODE)
> +            continue;
> +
> +        if (!def->file &&
> +            xmlStrEqual(cur->name, BAD_CAST "source")) {
> +
> +            int backingtype = def->type;
> +
> +            if (backingtype < 0)
> +                backingtype = VIR_DOMAIN_DISK_TYPE_FILE;

So far, def->type is never parsed, so the @backingtype is always
VIR_DOMAIN_DISK_TYPE_FILE. Mmm, okay.

> +
> +            if (virDomainDiskSourceDefParse(cur,
> +                                            backingtype,
> +                                            &def->file,
> +                                            NULL,
> +                                            NULL,
> +                                            NULL,
> +                                            NULL) < 0)
> +                goto cleanup;
> +
> +        } else if (!def->format &&
> +                   xmlStrEqual(cur->name, BAD_CAST "driver")) {
> +            char *driver = virXMLPropString(cur, "type");
> +            def->format = virStorageFileFormatTypeFromString(driver);
> +            if (def->format <= 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unknown disk snapshot driver '%s'"),
> +                               driver);
>                  VIR_FREE(driver);
> +                goto cleanup;
>              }
> +            VIR_FREE(driver);
>          }
> -        cur = cur->next;
>      }
> 
>      if (!def->snapshot && (def->file || def->format))
> @@ -577,6 +592,8 @@ static void
>  virDomainSnapshotDiskDefFormat(virBufferPtr buf,
>                                 virDomainSnapshotDiskDefPtr disk)
>  {
> +    int type = disk->type;
> +
>      if (!disk->name)
>          return;
> 
> @@ -584,6 +601,13 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
>      if (disk->snapshot > 0)
>          virBufferAsprintf(buf, " snapshot='%s'",
>                            virDomainSnapshotLocationTypeToString(disk->snapshot));
> +
> +    if (type < 0)
> +        type = VIR_DOMAIN_DISK_TYPE_FILE;
> +    else
> +        virBufferAsprintf(buf, " type='%s'",
> +                          virDomainDiskTypeToString(type));
> +

This is a very thin line between no XML change and outputting a new
'type' attribute into XML. Doesn't make much sense now, but I see this
is to be changed in 25/27. Anyway, I think it would be better to save
these small snippets up till then.

>      if (!disk->file && disk->format == 0) {
>          virBufferAddLit(buf, "/>\n");
>          return;
> @@ -591,12 +615,14 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf,
> 
>      virBufferAddLit(buf, ">\n");
> 
> -    virBufferAdjustIndent(buf, 6);
>      if (disk->format > 0)
> -        virBufferEscapeString(buf, "<driver type='%s'/>\n",
> +        virBufferEscapeString(buf, "      <driver type='%s'/>\n",
>                                virStorageFileFormatTypeToString(disk->format));
> -    virBufferEscapeString(buf, "<source file='%s'/>\n", disk->file);
> -    virBufferAdjustIndent(buf, -6);
> +    virDomainDiskSourceDefFormatInternal(buf,
> +                                         type,
> +                                         disk->file,
> +                                         0, 0, 0, NULL, 0, NULL, NULL, 0);
> +
>      virBufferAddLit(buf, "    </disk>\n");
>  }
> 
> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
> index ff3dca2..241d63c 100644
> --- a/src/conf/snapshot_conf.h
> +++ b/src/conf/snapshot_conf.h
> @@ -51,6 +51,7 @@ struct _virDomainSnapshotDiskDef {
>      char *name; /* name matching the <target dev='...' of the domain */
>      int index; /* index within snapshot->dom->disks that matches name */
>      int snapshot; /* enum virDomainSnapshotLocation */
> +    int type; /* enum virDomainDiskType */
>      char *file; /* new source file when snapshot is external */
>      int format; /* enum virStorageFileFormat */
>  };
> 

ACK to the rest.

Michal