1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/9] qemu: Support block pull/commit jobs with blockdev (blockdev-add saga)
  4. View patch

[libvirt] [PATCH 4/9] qemu: blockjob: Register disk->mirror with a job only when required

Peter Krempa posted 9 patches 6 years, 6 months ago
[libvirt] [PATCH 4/9] qemu: blockjob: Register disk->mirror with a job only when required
Posted by Peter Krempa 6 years, 6 months ago 
Commit job for example references existing storage in the <mirror>
subelement thus tracking it separately could lead into problems.

This patch introduces qemuBlockJobDiskRegisterMirror which registers the
mirror chain separately only for job which require it. This also comes
with remembering that in the status XML.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_blockjob.c                      | 22 ++++++++++++++++++-
 src/qemu/qemu_blockjob.h                      |  4 ++++
 src/qemu/qemu_domain.c                        | 14 +++++++++++-
 .../blockjob-blockdev-in.xml                  |  2 +-
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index ac7b3a0aef..a3109d3934 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -126,6 +126,9 @@ qemuBlockJobDataNew(qemuBlockJobType type,
  *
  * This function registers @job with @disk and @vm and records it into the status
  * xml (if @savestatus is true).
+ *
+ * Note that if @job also references a separate chain e.g. for disk mirroring
+ * qemuBlockJobDiskRegisterMirror should be used.
  */
 int
 qemuBlockJobRegister(qemuBlockJobDataPtr job,
@@ -143,7 +146,6 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job,
     if (disk) {
         job->disk = disk;
         job->chain = virObjectRef(disk->src);
-        job->mirrorChain = virObjectRef(disk->mirror);
         QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job);
     }

@@ -205,6 +207,24 @@ qemuBlockJobDiskNew(virDomainObjPtr vm,
 }


+/**
+ * qemuBlockJobDiskRegisterMirror:
+ * @job: block job to register 'mirror' chain on
+ *
+ * In cases when the disk->mirror attribute references a separate storage chain
+ * such as for block-copy, this function registers it with the job. Note
+ * that this function does not save the status XML and thus must be used before
+ * qemuBlockJobRegister or qemuBlockJobStarted to properly track the chain
+ * in the status XML.
+ */
+void
+qemuBlockJobDiskRegisterMirror(qemuBlockJobDataPtr job)
+{
+    if (job->disk)
+        job->mirrorChain = virObjectRef(job->disk->mirror);
+}
+
+
 /**
  * qemuBlockJobDiskGetJob:
  * @disk: disk definition
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index 47bdc54b2b..3299207610 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -110,6 +110,10 @@ qemuBlockJobDiskNew(virDomainObjPtr vm,
                     const char *jobname)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);

+void
+qemuBlockJobDiskRegisterMirror(qemuBlockJobDataPtr job)
+    ATTRIBUTE_NONNULL(1);
+
 qemuBlockJobDataPtr
 qemuBlockJobDiskGetJob(virDomainDiskDefPtr disk)
     ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e7f28aa2b8..c508f55287 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2367,7 +2367,10 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload,
     virBufferEscapeString(&childBuf, "<errmsg>%s</errmsg>", job->errmsg);

     if (job->disk) {
-        virBufferEscapeString(&childBuf, "<disk dst='%s'/>\n", job->disk->dst);
+        virBufferEscapeString(&childBuf, "<disk dst='%s'", job->disk->dst);
+        if (job->mirrorChain)
+            virBufferAddLit(&childBuf, " mirror='yes'");
+        virBufferAddLit(&childBuf, "/>\n");
     } else {
         if (job->chain &&
             qemuDomainObjPrivateXMLFormatBlockjobFormatChain(&chainsBuf,
@@ -2806,6 +2809,7 @@ qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,
     int state = QEMU_BLOCKJOB_STATE_FAILED;
     VIR_AUTOFREE(char *) diskdst = NULL;
     VIR_AUTOFREE(char *) newstatestr = NULL;
+    VIR_AUTOFREE(char *) mirror = NULL;
     int newstate = -1;
     bool invalidData = false;
     xmlNodePtr tmp;
@@ -2840,6 +2844,10 @@ qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,
         !(disk = virDomainDiskByName(vm->def, diskdst, false)))
         invalidData = true;

+    if ((mirror = virXPathString("string(./disk/@mirror)", ctxt)) &&
+        STRNEQ(mirror, "yes"))
+        invalidData = true;
+
     if (!disk && !invalidData) {
         if ((tmp = virXPathNode("./chains/disk", ctxt)) &&
             !(job->chain = qemuDomainObjPrivateXMLParseBlockjobChain(tmp, ctxt, xmlopt)))
@@ -2854,6 +2862,10 @@ qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,
     job->newstate = newstate;
     job->errmsg = virXPathString("string(./errmsg)", ctxt);
     job->invalidData = invalidData;
+    job->disk = disk;
+
+    if (mirror)
+        qemuBlockJobDiskRegisterMirror(job);

     if (qemuBlockJobRegister(job, vm, disk, false) < 0)
         return -1;
diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
index 5b9777ca71..7b9282d059 100644
--- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
+++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
@@ -235,7 +235,7 @@
   <nodename index='0'/>
   <blockjobs active='yes'>
     <blockjob name='drive-virtio-disk0' type='copy' state='ready'>
-      <disk dst='vda'/>
+      <disk dst='vda' mirror='yes'/>
     </blockjob>
     <blockjob name='test-orphan-job0' type='copy' state='ready'>
       <chains>
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] qemu: blockjob: Register disk->mirror with a job only when required
Posted by Eric Blake 6 years, 6 months ago 
On 7/24/19 4:07 PM, Peter Krempa wrote:
> Commit job for example references existing storage in the <mirror>
> subelement thus tracking it separately could lead into problems.

Grammar is hard to parse, maybe:

The <mirror> subelement is used in two ways: in a commit job to point to
existing storage, and in a block-copy job to point to additional
storage. We need a way to track only the distinct storage.

> 
> This patch introduces qemuBlockJobDiskRegisterMirror which registers the
> mirror chain separately only for job which require it. This also comes

for jobs which

> with remembering that in the status XML.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---

> +++ b/src/qemu/qemu_blockjob.c
> @@ -126,6 +126,9 @@ qemuBlockJobDataNew(qemuBlockJobType type,
>   *
>   * This function registers @job with @disk and @vm and records it into the status
>   * xml (if @savestatus is true).
> + *
> + * Note that if @job also references a separate chain e.g. for disk mirroring
> + * qemuBlockJobDiskRegisterMirror should be used.

a separate chain, e.g. for disk mirroring, then qemuBlock...

> +++ b/src/qemu/qemu_domain.c
> @@ -2367,7 +2367,10 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload,
>      virBufferEscapeString(&childBuf, "<errmsg>%s</errmsg>", job->errmsg);
> 
>      if (job->disk) {
> -        virBufferEscapeString(&childBuf, "<disk dst='%s'/>\n", job->disk->dst);
> +        virBufferEscapeString(&childBuf, "<disk dst='%s'", job->disk->dst);
> +        if (job->mirrorChain)
> +            virBufferAddLit(&childBuf, " mirror='yes'");

Is there any RNG grammar tweak needed to permit this? Or is the private
XML block internal only (if the user can never see it via
virXYZGetXMLDesc, then they have no reason to submit it during
virXYZCreate, so we have no reason to teach the RNG validation to ignore
it).

Wording fixes are minor, so
ACK

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] qemu: blockjob: Register disk->mirror with a job only when required
Posted by Peter Krempa 6 years, 6 months ago 
On Wed, Jul 24, 2019 at 17:02:46 -0500, Eric Blake wrote:
> On 7/24/19 4:07 PM, Peter Krempa wrote:
> > Commit job for example references existing storage in the <mirror>
> > subelement thus tracking it separately could lead into problems.
> 
> Grammar is hard to parse, maybe:
> 
> The <mirror> subelement is used in two ways: in a commit job to point to
> existing storage, and in a block-copy job to point to additional
> storage. We need a way to track only the distinct storage.
> 
> > 
> > This patch introduces qemuBlockJobDiskRegisterMirror which registers the
> > mirror chain separately only for job which require it. This also comes
> 
> for jobs which
> 
> > with remembering that in the status XML.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> 
> > +++ b/src/qemu/qemu_blockjob.c
> > @@ -126,6 +126,9 @@ qemuBlockJobDataNew(qemuBlockJobType type,
> >   *
> >   * This function registers @job with @disk and @vm and records it into the status
> >   * xml (if @savestatus is true).
> > + *
> > + * Note that if @job also references a separate chain e.g. for disk mirroring
> > + * qemuBlockJobDiskRegisterMirror should be used.
> 
> a separate chain, e.g. for disk mirroring, then qemuBlock...
> 
> > +++ b/src/qemu/qemu_domain.c
> > @@ -2367,7 +2367,10 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload,
> >      virBufferEscapeString(&childBuf, "<errmsg>%s</errmsg>", job->errmsg);
> > 
> >      if (job->disk) {
> > -        virBufferEscapeString(&childBuf, "<disk dst='%s'/>\n", job->disk->dst);
> > +        virBufferEscapeString(&childBuf, "<disk dst='%s'", job->disk->dst);
> > +        if (job->mirrorChain)
> > +            virBufferAddLit(&childBuf, " mirror='yes'");
> 
> Is there any RNG grammar tweak needed to permit this? Or is the private
> XML block internal only (if the user can never see it via
> virXYZGetXMLDesc, then they have no reason to submit it during
> virXYZCreate, so we have no reason to teach the RNG validation to ignore
> it).

The users can't see it (unless poking into internal files) and for some
weird reason we never implemented RNG for the status XML part.
I ran with it for consistency.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.