Re: [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]