On Mon, Apr 09, 2012 at 21:52:16 -0600, Eric Blake wrote:
> QUESTION: should we parse and ignore <mirror> on input, rather than
> rejecting it? By rejecting it, I can't add a unit test, since the
> unit test framework currently doesn't expose a way to trigger
> internal parsing.
>
> In order to track a block copy job across libvirtd restarts, we
> need to save internal XML that tracks the name of the file
> holding the mirror. Displaying this name in dumpxml might also
> be useful to the user, even if we don't yet have a way to (re-)
> start a domain with mirroring enabled up front. This is done
> with a new <mirror> sub-element to <disk>, as in:
>
> <disk type='file' device='disk'>
> <driver name='qemu' type='raw'/>
> <source file='/var/lib/libvirt/images/original.img'/>
> <mirror file='/var/lib/libvirt/images/copy.img' format='qcow2'/>
> ...
> </disk>
However, this would mean, the XML from virDomainGetXMLDesc() cannot be
directly used for defining new domain. I think there are two possible ways to
go. Either output <mirror> the way you did it and ignore it on input
(similarly to what we do with aliases or SELinux labels) or put the mirror
element out of domain XML and put it into state XML only.
> * docs/schemas/domaincommon.rng (diskspec): Add diskMirror.
> * docs/formatdomain.html.in (elementsDisks): Document it.
> * src/conf/domain_conf.h (_virDomainDiskDef): New members.
> * src/conf/domain_conf.c (virDomainDiskDefFree): Clean them.
> (virDomainDiskDefParseXML): Parse them, but only internally.
> (virDomainDiskDefFormat): Output them.
> ---
> docs/formatdomain.html.in | 11 ++++++++++
> docs/schemas/domaincommon.rng | 19 +++++++++++++++--
> src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 5 ++++
> 4 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index a382d30..534c44b 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1296,6 +1296,17 @@
> </table>
> <span class="since">Since 0.9.7</span>
> </dd>
> + <dt><code>mirror</code></dt>
> + <dd>
> + This element is present if the hypervisor has started a block
> + copy operation (via the <code>virDomainBlockCopy</code> API),
> + where the mirror location in attribute <code>file</code> will
> + eventually have the same contents as the source, and with the
> + file format in attribute <code>format</code> (which might
> + differ from the format of the source). For now, this element
> + only valid in output; it is rejected on
> + input. <span class="since">Since 0.9.12</span>
> + </dd>
> <dt><code>target</code></dt>
> <dd>The <code>target</code> element controls the bus / device
> under which the disk is exposed to the guest
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 0cc04af..66c91a2 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -772,6 +772,9 @@
> <ref name="driver"/>
> </optional>
> <optional>
> + <ref name='diskMirror'/>
> + </optional>
> + <optional>
> <ref name="diskAuth"/>
> </optional>
> <ref name="target"/>
> @@ -1013,9 +1016,7 @@
> </element>
> </define>
> <!--
> - Disk may use a special driver for access. Currently this is
> - only defined for Xen for tap/aio and file, but will certainly be
> - extended in the future, and libvirt doesn't look for specific values.
> + Disk may use a special driver for access.
> -->
Unrelated but it's not worth a separate patch.
> <define name="driver">
> <element name="driver">
> @@ -3024,6 +3025,18 @@
> <empty/>
> </element>
> </define>
> + <define name='diskMirror'>
> + <element name='mirror'>
> + <attribute name='file'>
> + <ref name='absFilePath'/>
> + </attribute>
> + <optional>
> + <attribute name='format'>
> + <ref name="genericName"/>
> + </attribute>
> + </optional>
> + </element>
> + </define>
> <define name="diskAuth">
> <element name="auth">
> <attribute name="username">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c6b97e1..8899653 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -933,6 +933,8 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
> VIR_FREE(def->dst);
> VIR_FREE(def->driverName);
> VIR_FREE(def->driverType);
> + VIR_FREE(def->mirror);
> + VIR_FREE(def->mirrorFormat);
> VIR_FREE(def->auth.username);
> if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
> VIR_FREE(def->auth.secret.usage);
> @@ -3318,6 +3320,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> char *ioeventfd = NULL;
> char *event_idx = NULL;
> char *copy_on_read = NULL;
> + char *mirror = NULL;
> + char *mirrorFormat = NULL;
> char *devaddr = NULL;
> virStorageEncryptionPtr encryption = NULL;
> char *serial = NULL;
> @@ -3453,6 +3457,22 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> ioeventfd = virXMLPropString(cur, "ioeventfd");
> event_idx = virXMLPropString(cur, "event_idx");
> copy_on_read = virXMLPropString(cur, "copy_on_read");
> + } else if ((mirror == NULL) &&
> + (xmlStrEqual(cur->name, BAD_CAST "mirror"))) {
You could have also removed those extra () when copy&pasting the code :-)
> + if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) {
> + mirror = virXMLPropString(cur, "file");
> + if (!mirror) {
> + virDomainReportError(VIR_ERR_XML_ERROR, "%s",
> + _("mirror requires file name"));
> + goto error;
> + }
> + mirrorFormat = virXMLPropString(cur, "format");
> + } else {
> + virDomainReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Cannot handle disk mirror on "
> + "input yet"));
> + goto error;
> + }
> } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
> authUsername = virXMLPropString(cur, "username");
> if (authUsername == NULL) {
> @@ -3867,6 +3887,10 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> driverName = NULL;
> def->driverType = driverType;
> driverType = NULL;
> + def->mirror = mirror;
> + mirror = NULL;
> + def->mirrorFormat = mirrorFormat;
> + mirrorFormat = NULL;
> def->encryption = encryption;
> encryption = NULL;
> def->serial = serial;
> @@ -3882,6 +3906,12 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> !(def->driverName = strdup(caps->defaultDiskDriverName)))
> goto no_memory;
>
> +
> + if (def->mirror && !def->mirrorFormat &&
> + caps->defaultDiskDriverType &&
> + !(def->mirrorFormat = strdup(caps->defaultDiskDriverType)))
> + goto no_memory;
> +
> if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
> && virDomainDiskDefAssignAddress(caps, def) < 0)
> goto error;
> @@ -3907,6 +3937,8 @@ cleanup:
> VIR_FREE(authUsage);
> VIR_FREE(driverType);
> VIR_FREE(driverName);
> + VIR_FREE(mirror);
> + VIR_FREE(mirrorFormat);
> VIR_FREE(cachetag);
> VIR_FREE(error_policy);
> VIR_FREE(rerror_policy);
> @@ -10829,6 +10861,16 @@ virDomainDiskDefFormat(virBufferPtr buf,
> }
> }
>
> + /* For now, mirroring is currently output-only: we always output
> + * it, but refuse to parse it on input except for internal parse
> + * on libvirtd restart. */
> + if (def->mirror) {
> + virBufferEscapeString(buf, " <mirror file='%s'", def->mirror);
> + if (def->mirrorFormat)
> + virBufferAsprintf(buf, " format='%s'", def->mirrorFormat);
> + virBufferAddLit(buf, "/>\n");
> + }
> +
> virBufferAsprintf(buf, " <target dev='%s' bus='%s'",
> def->dst, bus);
> if ((def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 0eed60e..abc953d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -563,6 +563,10 @@ struct _virDomainDiskDef {
> char *driverName;
> char *driverType;
>
> + char *mirror;
> + char *mirrorFormat;
> + bool mirroring;
> +
> virDomainBlockIoTuneInfo blkdeviotune;
>
> char *serial;
> @@ -2125,6 +2129,7 @@ VIR_ENUM_DECL(virDomainDiskIo)
> VIR_ENUM_DECL(virDomainDiskSecretType)
> VIR_ENUM_DECL(virDomainDiskSnapshot)
> VIR_ENUM_DECL(virDomainDiskTray)
> +VIR_ENUM_DECL(virDomainDiskMirrorStage)
> VIR_ENUM_DECL(virDomainIoEventFd)
> VIR_ENUM_DECL(virDomainVirtioEventIdx)
> VIR_ENUM_DECL(virDomainDiskCopyOnRead)
This last hunk looks weired. Should it go into another patch perhaps?
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list