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

[John Ferlan]

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
> @@ -1759,6 +1759,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
>      /* clear previously used namespaces */
>      virBitmapFree(priv->namespaces);
>      priv->namespaces = NULL;
> +
> +    priv->reconnectNoActiveBlockjobs = false;

If this were a tristate, then this would be ABSENT (or 0)...

>  }
> 
> 
> @@ -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')

> +
> +    return virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL);
> +}
> +
> +
>  static int
>  qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
>                                virDomainObjPtr vm)
> @@ -1976,6 +1993,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
>      if (priv->chardevStdioLogd)
>          virBufferAddLit(buf, "<chardevStdioLogd/>\n");
> 
> +    if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0)
> +        return -1;
> +
>      return 0;
>  }
> 
> @@ -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);

> +
> +    VIR_FREE(active);
> +    return 0;
> +}
> +
> +
>  static int
>  qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>                               virDomainObjPtr vm,
> @@ -2282,6 +2318,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>      priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)",
>                                               ctxt) == 1;
> 
> +    if (qemuDomainObjPrivateXMLParseBlockjobs(priv, ctxt) < 0)
> +        goto error;
> +
>      return 0;
> 
>   error:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 1a47396ab..82248b56f 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -320,6 +320,9 @@ struct _qemuDomainObjPrivate {
> 
>      /* If true virtlogd is used as stdio handler for character devices. */
>      bool chardevStdioLogd;
> +
> +    /* if true there were no active blockjobs at reconnect to the VM */
> +    bool reconnectNoActiveBlockjobs;

virTristateBool

>  };
> 
>  # define QEMU_DOMAIN_PRIVATE(vm)	\
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 4e21f5825..5339f673f 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -34,6 +34,7 @@ struct testInfo {
>      char *outInactiveName;
> 
>      virBitmapPtr activeVcpus;
> +    bool blockjobs;
> 
>      virQEMUCapsPtr qemuCaps;
>  };
> @@ -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;
>  }
> 
> @@ -124,6 +136,17 @@ testGetStatuXMLPrefixVcpus(virBufferPtr buf,
>  }
> 
> 
> +static void
> +testGetStatusXMLAddBlockjobs(virBufferPtr buf,
> +                             const struct testInfo *data)
> +{
> +    if (data->blockjobs)
> +        virBufferAddLit(buf, "<blockjobs active='yes'/>\n");
> +    else
> +        virBufferAddLit(buf, "<blockjobs active='no'/>\n");

It's just a test, but you could use the same ToString logic.

What you have works, but it's confusing to see double negatives. Always
have to read them much more closely.


John

> +}
> +
> +
>  static char *
>  testGetStatusXMLPrefix(const struct testInfo *data)
>  {
> @@ -136,12 +159,31 @@ testGetStatusXMLPrefix(const struct testInfo *data)
> 
>      virBufferAddStr(&buf, testStatusXMLPrefixBodyStatic);
> 
> +    testGetStatusXMLAddBlockjobs(&buf, data);
> +
>      virBufferAdjustIndent(&buf, -2);
> 
>      return virBufferContentAndReset(&buf);
>  }
> 
> 
> +static int
> +testProcessStatusXML(virDomainObjPtr vm)
> +{
> +    size_t i;
> +
> +    /* fix the private 'blockjob' flag for disks */
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +        qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +
> +        diskPriv->blockjob = !!disk->mirror;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  testCompareStatusXMLToXMLFiles(const void *opaque)
>  {
> @@ -200,6 +242,10 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
>          goto cleanup;
>      }
> 
> +    /* process the definition if necessary */
> +    if (testProcessStatusXML(obj) < 0)
> +        goto cleanup;
> +
>      /* format it back */
>      if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL,
>                                        VIR_DOMAIN_DEF_FORMAT_SECURE))) {
>