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