[libvirt] [PATCH 3/5] qemu: domain: Store and restore autoCpuset to status XML

[Peter Krempa]

On Thu, Jul 20, 2017 at 14:12:44 +0200, Andrea Bolognani wrote:
> On Wed, 2017-07-12 at 15:44 +0200, Peter Krempa wrote:
> > @@ -1765,20 +1765,30 @@ qemuDomainObjPtrivateXMLFormatAutomaticPlacement(virBufferPtr buf,
> >                                                   qemuDomainObjPrivatePtr priv)
> >  {
> [...]
> > -    virBufferAsprintf(buf, "<numad nodeset='%s'/>\n", nodeset);
> > +    if (priv->autoCpuset &&
> > +        !((cpuset = virBitmapFormat(priv->autoCpuset))))
> > +        goto cleanup;
> 
> The previous implementation didn't format the <numad>
> element at all if there was nodeset, whereas the new one
> will always format it. You could add
> 
>     if (!nodeset && !cpuset)
>         goto cleanup;

If you call virBitmapFormat on an empty or NULL bitmap you still get a
(empty) string allocated so that condition is basically identical to the
one that's already there due to how the bitmaps are formatted:

    if (!priv->autoNodeset && !priv->autoCpuset)
        return 0;

    if (priv->autoNodeset &&
        !((nodeset = virBitmapFormat(priv->autoNodeset))))
        goto cleanup;

    if (priv->autoCpuset &&
        !((cpuset = virBitmapFormat(priv->autoCpuset))))
        goto cleanup;


> here to restore that behavior, but maybe you just decided
> it's not worth it. Just felt like I should point that out.
> 
> > +    virBufferAddLit(buf, "<numad");
> > +    virBufferEscapeString(buf, " nodeset='%s'", nodeset);
> > +    virBufferEscapeString(buf, " cpuset='%s'", cpuset);
> 
> I had to look up the implementation of virBufferEscapeString()
> to figure out that nodeset or cpuset possibly being NULL is
> handled automatically by that function, which I found to be a
> pretty surprising behavior. Not a problem with your patch
> though :)
> 
> > @@ -1958,11 +1968,13 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver,
> >  {
> >      virCapsPtr caps = NULL;
> >      char *nodeset;
> > +    char *cpuset;
> >      int ret = -1;
> > 
> >      nodeset = virXPathString("string(./numad/@nodeset)", ctxt);
> > +    cpuset = virXPathString("string(./numad/@cpuset)", ctxt);
> > 
> > -    if (!nodeset)
> > +    if (!nodeset && !cpuset)
> >          return 0;
> 
> I don't think you want this hunk.
> 
> With the new condition, you will perform an early exit only
> if both nodeset and cpuset are NULL; if nodeset is NULL but
> cpuset isn't, the first call to virBitmapParse() a few lines
> below will error out.

That shouldn't ever happen (tm) :D

I'll can add a condition that if nodeset is not in the XML the parsing
will be skipped. So in that case only cpuset can be present (for future
use).

I'll also add a note that accessing autoNodeset in the else branch of if
(cpuset) is safe.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170720/ca142b69/attachment-0001.sig>