On 28/02/13 03:23, Eric Blake wrote:
> On 02/27/2013 07:57 PM, TJ wrote:
>> From: TJ <linux@xxxxxx>
>>
>> Maintain backwards XML compatibility by assuming existing default values
>> and only adding the additional XML properties if settings are not
>> default.
>>
>> Signed-off-by: TJ <linux@xxxxxx>
>> ---
>> src/conf/network_conf.c | 28 ++++++++++++++++++++++++----
>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>
>
>> + def->dhcp_enabled = true;
>> + if ((tmp = virXMLPropString(node, "enabled"))) {
>> + def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled;
>
> Yuck. This lets a user pass in trailing garbage. Use STREQ, not strncmp.
>
> For that matter, assigning def->dhcp_enabled to itself looks odd. I'd
> probably write:
>
> def->dhcp_enabled = true;
> if ((tmp = virXMLPropString(node, "enabled"))) {
> if (STREQ(tmp, "no"))
> def->dhcp_enabled = false;
> VIR_FREE(tmp);
>
> so that it doesn't look so screwy.
I knew there was probably a better 'libvirt' style but couldn't find examples when I was looking for them.
>> + !def->dhcp_enabled || def->dhcp_relay) {
>> int ii;
>> - virBufferAddLit(buf, "<dhcp>\n");
>> + virBufferAddLit(buf, "<dhcp");
>> + if (!def->dhcp_enabled)
>> + virBufferAddLit(buf, " enabled='no'");
>> + if (def->dhcp_relay)
>> + virBufferAddLit(buf, " relay='yes'");
>> + virBufferAddLit(buf, ">\n");
>> +
>> virBufferAdjustIndent(buf, 2);
>>
>> - for (ii = 0 ; ii < def->nranges ; ii++) {
>> + for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
>
> This line is a spurious change.
Ahh yes. I introduced that to catch a bug found using gdb when nranges == 0 in the mistaken thinking that the loop would do at least one iteration. Later I realised the bug was caused by another issue
entirely but forgot to revert that change.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list