Re: [libvirt] [PATCH 5/6] qemu: domain: Mark if no blockjobs are active in the status XML

On 10/04/2017 07:59 AM, Peter Krempa wrote: > Note when no blockjobs are running in the status XML so that we know > that the backing chain will not change until we reconnect. > --- > src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_domain.h | 3 +++ > tests/qemuxml2xmltest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 88 insertions(+) > Was there a special reason to use the double negative of "reconnectNoActiveBlockjobs" instead of "reconnectActiveBlockjobs" or "reconnectDetermineDiskChain". If it's not present in the status XML, then the default is there are active block jobs or more succinctly as the next patch shows - we need to determine the backing chain when either active == true or it's not defined in the status XML (a/k/a VIR_TRISTATE_BOOL_ABSENT). Also since the definition is much lower and making this comment now makes subsequent comments easier to understand, what not make it a virTristateBool since that's how you parse it (essentially)?

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index b202d02f9..2bc8f38dc 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c

> @@ -1854,6 +1856,21 @@ qemuDomainObjPrivateXMLFormatAutomaticPlacement(virBufferPtr buf, > } > > > +static int > +qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf, > + virDomainObjPtr vm) > +{ > + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; > + > + if (!qemuDomainHasBlockjob(vm, false)) > + virBufferAddLit(&attrBuf, " active='no'"); > + else > + virBufferAddLit(&attrBuf, " active='yes'"); And these would use virTristateSwitchTypeToString of course. Whether you keep the same order to take "active='no'" == ABSENT is your call (that is, no special need to format 'no')

> @@ -2067,6 +2087,22 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt, > } > > > +static int > +qemuDomainObjPrivateXMLParseBlockjobs(qemuDomainObjPrivatePtr priv, > + xmlXPathContextPtr ctxt) > +{ > + char *active; > + > + if ((active = virXPathString("string(./blockjobs/@active)", ctxt))) { > + if (virTristateBoolTypeFromString(active) == VIR_TRISTATE_BOOL_NO) > + priv->reconnectNoActiveBlockjobs = true; > + } Then, just (in the format that mkletzan prefers ;-)): active = virXPathString("string(./blockjobs/@active)", ctxt); if (active) priv->NAME = virTristateBoolTypeFromString(active);

> @@ -43,11 +44,22 @@ qemuXML2XMLActivePreFormatCallback(virDomainDefPtr def, > const void *opaque) > { > struct testInfo *info = (struct testInfo *) opaque; > + size_t i; > > /* store vCPU bitmap so that the status XML can be created faithfully */ > if (!info->activeVcpus) > info->activeVcpus = virDomainDefGetOnlineVcpumap(def); > > + info->blockjobs = false; > + > + /* remember whether we have mirror jobs */ > + for (i = 0; i < def->ndisks; i++) { > + if (def->disks[i]->mirror) { Slightly different check than qemuDomainHasBlockjob, but this is just a test.

> + info->blockjobs = true; > + break; > + } > + } > + > return 0; > } >