On 06/06/14 00:52, Eric Blake wrote:
> A future patch is going to wire up qemu active block commit jobs;
> but as they have similar events and are canceled/pivoted in the
> same way as block copy jobs, it is easiest to track all bookkeeping
> for the commit job by reusing the <mirror> element. This patch
> adds domain XML to track which job was responsible for creating a
> mirroring situation, and adds a job='copy' attribute to all
> existing uses of <mirror>. Along the way, it also massages the
> qemu monitor backend to read the new field in order to generate
> the correct type of libvirt job (even though it requires a
> future patch to actually cause a qemu event that can be reported
> as an active commit).
>
> * docs/schemas/domaincommon.rng: Enhance schema.
> * docs/formatdomain.html.in: Document it.
> * src/conf/domain_conf.h (_virDomainDiskDef): Add a field.
> * src/conf/domain_conf.c (virDomainBlockJobType): String conversion.
> (virDomainDiskDefParseXML): Parse job type.
> (virDomainDiskDefFormat): Output job type.
> * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Distinguish
> active from regular commit.
> * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Set job type.
> (qemuDomainBlockPivot, qemuDomainBlockJobImpl): Clean up job type
> on completion.
> * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml:
> Update tests.
> * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Likewise.
> * tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml: New
> file.
> * tests/qemuxml2xmltest.c (mymain): Drive new test.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 15 +++++----
> docs/schemas/domaincommon.rng | 6 ++++
> src/conf/domain_conf.c | 31 ++++++++++++++++--
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_driver.c | 3 ++
> src/qemu/qemu_process.c | 18 +++++++----
> .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++++++++++++++
> .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +--
> .../qemuxml2xmlout-disk-mirror-old.xml | 4 +--
> tests/qemuxml2xmltest.c | 1 +
> 10 files changed, 100 insertions(+), 20 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 1b6ced8..3bf1f6d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1878,13 +1878,16 @@
> </dd>
> <dt><code>mirror</code></dt>
> <dd>
> - This element is present if the hypervisor has started a block
> - copy operation (via the <code>virDomainBlockRebase</code>
> - API), where the mirror location in the <code>source</code>
> - sub-element will eventually have the same contents as the
> - source, and with the file format in the
> + This element is present if the hypervisor has started a
> + long-running block job operation, where the mirror location in
> + the <code>source</code> sub-element will eventually have the
> + same contents as the source, and with the file format in the
> sub-element <code>format</code> (which might differ from the
> - format of the source). The details of the <code>source</code>
> + format of the source). The <code>job</code> attribute
> + mentions which API started the operation ("copy"
> + for the <code>virDomainBlockRebase</code> API, or "commit"
> + for the <code>virDomainBlockCommit</code> API). The details of
> + the <code>source</code>
Although this is HTML the rest of the paragraph should be re-flowed.
> sub-element are determined by the <code>type</code> attribute
> of the mirror, similar to what is done for the
> overall <code>disk</code> device element. If
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2b7dc26..92009ce 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -760,6 +760,11 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST,
> "unmap",
> "ignore")
>
> +/* Internal mapping of block job types associated with <mirror> */
Probably worth mentioning, that the values will be present in the XML
and shouldn't be changed.
> +VIR_ENUM_DECL(virDomainBlockJob)
> +VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST,
> + "", "", "copy", "", "active-commit")
> +
> #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE
> #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
>
> @@ -15191,9 +15212,13 @@ virDomainDiskDefFormat(virBufferPtr buf,
> * new style similar to backingStore, even though the parser
> * code still accepts old style across libvirtd upgrades. */
> if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) {
> - virBufferAsprintf(buf, "<mirror type='%s'%s>\n",
> - virStorageTypeToString(def->mirror->type),
> - def->mirroring ? " ready='yes'" : "");
> + virBufferAsprintf(buf, "<mirror type='%s'",
> + virStorageTypeToString(def->mirror->type));
> + virBufferEscapeString(buf, " job='%s'",
> + virDomainBlockJobTypeToString(def->mirrorJob));
> + if (def->mirroring)
> + virBufferAddLit(buf, " ready='yes'");
> + virBufferAddLit(buf, ">\n");
Hmmm in one of your previous patches you've changed the lines above to
be part of the deleted virBufferAsprintf and now you are returning them.
But not worth changing the patch due to this.
> virBufferAdjustIndent(buf, 2);
> if (def->mirror->format)
> virBufferEscapeString(buf, "<format type='%s'/>\n",
ACK with the two comment/docs nits addressed.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list