Re: [libvirt] [PATCH v2 1/3] conf: Format numatune XML correctly while placement is none
On 07/10/2012 04:00 AM, Osier Yang wrote:
On 2012年07月10日 17:41, Daniel P. Berrange wrote:
> On Tue, Jul 10, 2012 at 05:31:17PM +0800, Osier Yang wrote:
>> ping?
>>
>> On 2012年06月25日 12:28, Osier Yang wrote:
>>> setNumaParameters tunes the numa setting using cgroup, it's another
>>> entry except libnuma/numad for numa tuning. And it doesn't set the
>>> placement, and further more, the formating codes doesn't take this
>>> into consideration.
>>>
+ } else {
+ /* Should not hit here. */
+ virBufferAddLit(buf, "/>\n");
If we can't get here, then we have a bigger bug. Fixing the output is
wrong if we have an earlier bug generating an inconsistent struct in the
first place.
>
> The fact that this bug existed shows that the test suite for the XML
> parser is incomplete. Please resubmit with an extra test XML datafile
> for the test suite to validate this scenario.
I agree.
>
we already has good enough XMLs for the test suite:
% ls tests/qemuxml2argvdata/qemuxml2argv-numa
qemuxml2argv-numad.args qemuxml2argv-numad-auto-vcpu-static-numatune.xml
qemuxml2argv-numad-auto-memory-vcpu-cpuset.args
qemuxml2argv-numad-static-memory-auto-vcpu.args
qemuxml2argv-numad-auto-memory-vcpu-cpuset.xml
qemuxml2argv-numad-static-memory-auto-vcpu.xml
qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.args
qemuxml2argv-numad-static-vcpu-no-numatune.xml
qemuxml2argv-numad-auto-memory-vcpu-no-cpuset-and-placement.xml
qemuxml2argv-numad.xml
qemuxml2argv-numad-auto-vcpu-no-numatune.xml
qemuxml2argv-numatune-memory.args
qemuxml2argv-numad-auto-vcpu-static-numatune.args
qemuxml2argv-numatune-memory.xml
These may have used <numatune>, but none of them used it in the manner
that would have been detected by round trip parsing. Ergo, we are
missing a test, either a test that invalid input data is rejected
earlier on, or a test that when input data omits information, then round
trip handing of that data doesn't get the output into an inconsistent
state when it adds in the defaults for the omitted information.
The problem is we have two entries to change the numatune config,
and they share the same XML syntax (btw, I was thinking it's a
bad idea to do so, it could just cause crasy results).
Which two entries are you thinking of? Maybe it is a bad design
decision, but we're stuck with it, so we should at least support what we
have documented.
XML parser
actually ensures the placement mode can be always set with either
'static' or 'auto', but API via cgroup don't set placement mode
as placement is meaningless for it, so IMHO no need to add
XMLs to test the parser, instead we need to add tests to test
the API.
The TCK is a great place to test the API. But maybe the real bug here
is that if you hotplug a request for a numatune and it goes through
cgroup, then we should be updating the conf struct to reflect either
static or auto, even though cgroup doesn't directly care, so that the
output function then works in all cases.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)