[libvirt] [PATCH v2 3/5] Network: Move dnsmasqContext creation to networkSaveDnsmasqHostsfile() and pass to dnsmasq only if applicable

[Laine Stump]

On 04/01/2011 06:45 AM, Michal Novotny wrote:
> Make: passed
> Make check: passed
> Make syntax-check: passed
>
> Hi,
> this is the patch to move the dnsmasqContext creation to the
> networkSaveDnsmasqHostsfile() function and also it passes the
> hosts-file and addn-hosts to the file only if applicable, i.e.
> if it's already set.
>
> Originally I wanted to call the DhcpHostsfile and AddnHostsfile
> creation on the first call to dnsmasqAddDhcpHost/dnsmasqAddHost
> however that way I would have kept track of the path name to
> be generated which would require storing network name and config
> directory somewhere in the structure and that's why I changed
> it to simple approach used in this patch.
>
> Michal


This all looks straightforward. The only comment I have (other than to 
sanitize the commit comment) would be that some day in the future I 
would like to be able to specify static dhcp hosts under multiple IP 
addresses within a network (dnsmasq can handle this, it just needs a bit 
of extra trickery in the commandline), so anything you do to make that 
easier would be good. (but as long as you don't make it any more 
difficult than it already is, I'll ACK this when you resubmit the series).


> Signed-off-by: Michal Novotny<minovotn at redhat.com>
> ---
>   src/network/bridge_driver.c |   38 +++++++++++++++++++++-----------------
>   1 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index b6ce39d..41b14f9 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -420,13 +420,20 @@ networkShutdown(void) {
>   }
>
>
> -static int
> +static dnsmasqContext*
>   networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
> -                            dnsmasqContext *dctx,
> +                            char *name,
>                               bool force)
>   {
>       unsigned int i;
>
> +    dnsmasqContext *dctx = dnsmasqContextNew(name,
> +                                             DNSMASQ_STATE_DIR);
> +    if (dctx == NULL) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
>       if (! force&&  virFileExists(dctx->hostsfile->path))
>           return 0;
>
> @@ -437,9 +444,14 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
>       }
>
>       if (dnsmasqSave(dctx)<  0)
> -        return -1;
> +        goto cleanup;
>
> -    return 0;
> +    return dctx;
> +
> +cleanup:
> +    dnsmasqContextFree(dctx);
> +
> +    return NULL;
>   }
>
>   static int
> @@ -451,6 +463,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>       int nbleases = 0;
>       int ii;
>       virNetworkIpDefPtr tmpipdef;
> +    dnsmasqContext *dctx = NULL;
>
>       /*
>        * NB, be careful about syntax for dnsmasq options in long format.
> @@ -572,18 +585,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>           if (ipdef->nranges || ipdef->nhosts)
>               virCommandAddArg(cmd, "--dhcp-no-override");
>
> -        if (ipdef->nhosts>  0) {
> -            dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
> -                                                     DNSMASQ_STATE_DIR);
> -            if (dctx == NULL) {
> -                virReportOOMError();
> -                goto cleanup;
> -            }
> -
> -            if (networkSaveDnsmasqHostsfile(ipdef, dctx, false) == 0) {
> +        if ((dctx = networkSaveDnsmasqHostsfile(ipdef, network->def->name, false))) {
> +            if (dctx->hostsfile->nhosts)
>                   virCommandAddArgPair(cmd, "--dhcp-hostsfile",
>                                        dctx->hostsfile->path);
> -            }
> +
>               dnsmasqContextFree(dctx);
>           }
>
> @@ -2203,11 +2209,9 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
>           }
>       }
>       if (ipv4def) {
> -        dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR);
> +        dnsmasqContext* dctx = networkSaveDnsmasqHostsfile(ipv4def, network->def->name, true);
>           if (dctx == NULL)
>               goto cleanup;
> -
> -        networkSaveDnsmasqHostsfile(ipv4def, dctx, true);
>           dnsmasqContextFree(dctx);
>       }
>