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

[Eric Blake]

On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ <linux at iam.tj>
> 
> 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 at iam.tj>
> ---
>  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.

> +        VIR_FREE(tmp);
> +    }
> +
> +    def->dhcp_relay = false;
> +    if ((tmp = virXMLPropString(node, "relay"))) {
> +        def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay;
> +	VIR_FREE(tmp);

Same comments.

> +    }
>  
>      cur = node->children;
>      while (cur != NULL) {
> @@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf,
>          virBufferEscapeString(buf, "<tftp root='%s' />\n",
>                                def->tftproot);
>      }
> -    if ((def->nranges || def->nhosts)) {
> +    if ((def->nranges || def->nhosts) || 

As long as you are touching this, you can drop the redundant ().

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

>              char *saddr = virSocketAddrFormat(&def->ranges[ii].start);
>              if (!saddr)
>                  goto error;
> @@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf,
>              VIR_FREE(saddr);
>              VIR_FREE(eaddr);
>          }
> -        for (ii = 0 ; ii < def->nhosts ; ii++) {
> +        for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) {

So is this one.

>              virBufferAddLit(buf, "<host ");
>              if (def->hosts[ii].mac)
>                  virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130227/141351af/attachment-0001.sig>