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.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
