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.