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

[Laine Stump]

On 02/27/2013 09: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(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 3fc01cf..259de0a 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -828,6 +828,19 @@ virNetworkDHCPDefParseXML(const char *networkName,
>  {
>  
>      xmlNodePtr cur;
> +    char *tmp = NULL;
> +
> +    def->dhcp_enabled = true;
> +    if ((tmp = virXMLPropString(node, "enabled"))) {
> +        def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled;
> +        VIR_FREE(tmp);
> +    }

See my earlier comment about the lack of a need for an enable attribute
for dhcp.

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

At the end of this function you should check for dhcp_relay and if it's
true, verify that nothing else was included in the <dhcp> element. It's
okay/desirable for the parser to ignore extra stuff if it just doesn't
understand it, or if it's unknown whether or not the backend driver
would accept it, but in this case it's certain that if you want a dhcp
relay, adding an <ip> element etc would be nonsensical, so we might as
well save the backend driver the trouble of checking for it.



>  
>      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) || 
> +         !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");
> +

Since relay='yes' necessarily mandates that there can be no subelements
(at least until such time as we figure out options we need to add for a
dhcp relay and add them in), you could be extra tidy by just closing the
element right here on a single line, then skipping the rest of the
function (the parser should have already validated that there was no
extra information in the case of relay='yes')


>          virBufferAdjustIndent(buf, 2);
>  
> -        for (ii = 0 ; ii < def->nranges ; ii++) {
> +        for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
>              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++) {
>              virBufferAddLit(buf, "<host ");
>              if (def->hosts[ii].mac)
>                  virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);