Decouple them by storing them in the XML separately rather than
regenerating them. This will simplify upcoming fixes.
---
src/qemu/qemu_domain.c | 32 +++++++++++++++++++++++++-------
tests/qemuxml2xmltest.c | 2 +-
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 92e5609ad..8c29db15f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1765,20 +1765,30 @@ qemuDomainObjPtrivateXMLFormatAutomaticPlacement(virBufferPtr buf,
qemuDomainObjPrivatePtr priv)
{
char *nodeset = NULL;
+ char *cpuset = NULL;
int ret = -1;
- if (!priv->autoNodeset)
+ if (!priv->autoNodeset && !priv->autoCpuset)
return 0;
- if (!(nodeset = virBitmapFormat(priv->autoNodeset)))
+ if (priv->autoNodeset &&
+ !((nodeset = virBitmapFormat(priv->autoNodeset))))
goto cleanup;
- virBufferAsprintf(buf, "<numad nodeset='%s'/>\n", nodeset);
+ if (priv->autoCpuset &&
+ !((cpuset = virBitmapFormat(priv->autoCpuset))))
+ goto cleanup;
+
+ virBufferAddLit(buf, "<numad");
+ virBufferEscapeString(buf, " nodeset='%s'", nodeset);
+ virBufferEscapeString(buf, " cpuset='%s'", cpuset);
+ virBufferAddLit(buf, "/>\n");
ret = 0;
cleanup:
VIR_FREE(nodeset);
+ VIR_FREE(cpuset);
return ret;
}
@@ -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;
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
@@ -1971,15 +1983,21 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(virQEMUDriverPtr driver,
if (virBitmapParse(nodeset, &priv->autoNodeset, caps->host.nnumaCell_max) < 0)
goto cleanup;
- if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps,
- priv->autoNodeset)))
- goto cleanup;
+ if (cpuset) {
+ if (virBitmapParse(cpuset, &priv->autoCpuset, VIR_DOMAIN_CPUMASK_LEN) < 0)
+ goto cleanup;
+ } else {
+ if (!(priv->autoCpuset = virCapabilitiesGetCpusForNodemask(caps,
+ priv->autoNodeset)))
+ goto cleanup;
+ }
ret = 0;
cleanup:
virObjectUnref(caps);
VIR_FREE(nodeset);
+ VIR_FREE(cpuset);
return ret;
}
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 79347671f..24d1eb33c 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -95,7 +95,7 @@ static const char testStatusXMLPrefixFooter[] =
" <device alias='net0'/>\n"
" <device alias='usb'/>\n"
" </devices>\n"
-" <numad nodeset='0-2'/>\n"
+" <numad nodeset='0-2' cpuset='1,3'/>\n"
" <libDir path='/tmp'/>\n"
" <channelTargetDir path='/tmp/channel'/>\n";
--
2.12.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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;
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.
But leaving the condition alone makes sure all scenarios are
handled successfully.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-07-20 at 15:46 +0200, Peter Krempa wrote: > > > - 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; Oh, you're right. Nevermind then. > > > - 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. Works for me :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.