[libvirt] [PATCH v2 1/3] conf: Format numatune XML correctly while placement is none

[Osier Yang]

On 2012年07月31日 01:37, Eric Blake wrote:
> 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.

We won't get here anyway, either fixing the output or the internal
inconsistent struct. If fixing the output, then just as this patch
did, checking if the nodeset is set (this prohibit the case which
doesn't set placement for internal struct - like setNumaParameters
does, the current output formating code doesn't take it into
consideration). If fixing setNumaParameters to set a placement even if
'placement' doesn't make sense for cgroup, we also can't get the
'else' branch.

  Fixing the output is
> wrong if we have an earlier bug generating an inconsistent struct in the
> first place.

Agreed.

>
>>>
>>> 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.

Yeah. The reason for why I think the testing xml datafiles are
enough is we considered nearly everything from parsing p.o.v.

  Ergo, we are
> missing a test, either a test that invalid input data is rejected
> earlier on,

And the xml datafiles cover this.

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.

Yes, we miss such a test. Which doesn't relates with the parsing,
it's monodirectional from the API -> internal struct -> output. So
no xml datafiles. That was why I didn't understand Daniel said
xml datafile is needed.

>
>>
>> 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?

One is libnuma, one is cgroup, so I meant the entries underlying.
they use same syntax, different implementation, and for same
driver. Which could just make things a mess. We should avoid
design like this in future.

   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.

Guess I forgot this apprently. :-)

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.

So, think we get a consensus: Add the test in TCK, and defaults
the internal struct so that it doesn't fall into inconsistence.
I will go this way and post v3.

Thanks for the opinions!

Regards,
Osier