On Wed, Oct 04, 2017 at 16:24:52 -0400, John Ferlan wrote:
>
>
> 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)?
Okay, I went with this in v2
> > 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')
Well. Actually the only interresting value is 'no'. If the value is
absent that might denote that libvirt was not recording the state, not
that there was no blockjob.
[...]
> > @@ -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);
Since you suggested to modify the type to virTristateBool, you can't
just assign it, also there can't be an inline check due to
virTristateBool being unsigned but virTristateBoolTypeFromString
returning -1.
I went with an intermediate integer and kept the style I've used since I
prefer it more.
> > @@ -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.
Obviously. We don't record diskpriv->blockjob in the XML thus that check
can't be used. Later when qemu will allow to reload state of blockjobs
it can be fixed to fully track blockjobs.
>
> > + info->blockjobs = true;
> > + break;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
