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

Re: [libvirt] [PATCHv2 5/9] network: reorganize dnsmasq and radvd config file / startup



On Tue, Sep 18, 2012 at 03:39:01AM -0400, Laine Stump wrote:
> This patch splits the starting of dnsmasq and radvd into multiple
> files, and adds new networkRefreshXX() and networkRestartXX()
> functions for each. These new functions are currently commented out
> because they won't be used until the next commit, and the compile options
> require all static functions to be used.
> 
> networkRefreshXX() - rewrites any file-based config for dnsmasq/radvd,
> and sends SIGHUP to the process to make it reread its config. If the
> program isn't already running, it's just started.
> 
> networkRestartXX() - kills the given program, waits for it to exit
> (see the comments in the function networkKillDaemon()), then calls
> networkStartXX().
> 
> This commit is here mostly as a checkpoint to verify no change in
> functional behavior after refactoring networkStartXX() functions to
> fit in with these new functions.
> ---
>  src/network/bridge_driver.c | 326 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 282 insertions(+), 44 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 8dbb050..5f7f31e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -468,6 +468,68 @@ networkShutdown(void) {
>  }
>  
>  
> +#if 0
> +/* currently unused, so it causes a build error unless we #if it out */
> +/* networkKillDaemon:
> + *
> + * kill the specified pid/name, and wait a bit to make sure it's dead.
> + */
> +static int
> +networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName)
> +{
> +    int ii, ret = -1;
> +    const char *signame = "TERM";
> +
> +    /* send SIGTERM, then wait up to 3 seconds for the process to
> +     * disappear, send SIGKILL, then wait for up to another 2
> +     * seconds. If that fails, log a warning and continue, hoping
> +     * for the best.
> +     */
> +    for (ii = 0; ii < 25; ii++) {
> +        int signum = 0;
> +        if (ii == 0)
> +            signum = SIGTERM;
> +        else if (ii == 15) {
> +            signum = SIGKILL;
> +            signame = "KILL";
> +        }
> +        if (kill(pid, signum) < 0) {
> +            if (errno == ESRCH) {
> +                ret = 0;
> +            } else {
> +                char ebuf[1024];
> +                VIR_WARN("Failed to terminate %s process %d "
> +                         "for network '%s' with SIG%s: %s",
> +                         daemonName, pid, networkName, signame,
> +                         virStrerror(errno, ebuf, sizeof(ebuf)));
> +            }
> +            goto cleanup;
> +        }
> +        /* NB: since networks have no reference count like
> +         * domains, there is no safe way to unlock the network
> +         * object temporarily, and so we can't follow the
> +         * procedure used by the qemu driver of 1) unlock driver
> +         * 2) sleep, 3) add ref to object 4) unlock object, 5)
> +         * re-lock driver, 6) re-lock object. We may need to add
> +         * that functionality eventually, but for now this
> +         * function is rarely used and, at worst, leaving the
> +         * network driver locked during this loop of sleeps will
> +         * have the effect of holding up any other thread trying
> +         * to make modifications to a network for up to 5 seconds;
> +         * since modifications to networks are much less common
> +         * than modifications to domains, this seems a reasonable
> +         * tradeoff in exchange for less code disruption.
> +         */
> +        usleep(20 * 1000);
> +    }
> +    VIR_WARN("Timed out waiting after SIG%s to %s process %d "
> +             "(network '%s')",
> +             signame, daemonName, pid, networkName);
> +cleanup:
> +    return ret;
> +}
> +#endif /* #if 0 */
> +
>  static int
>  networkBuildDnsmasqHostsfile(dnsmasqContext *dctx,
>                               virNetworkIpDefPtr ipdef,
> @@ -777,6 +839,12 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
>      int ret = -1;
>      dnsmasqContext *dctx = NULL;
>  
> +    if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) {
> +        /* no IPv6 addresses, so we don't need to run radvd */
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
>      if (virFileMakePath(NETWORK_PID_DIR) < 0) {
>          virReportSystemError(errno,
>                               _("cannot create directory %s"),
> @@ -840,50 +908,87 @@ cleanup:
>      return ret;
>  }
>  
> +#if 0
> +/* currently unused, so they cause a build error unless we #if them out */
> +/* networkRefreshDhcpDaemon:
> + *  Update dnsmasq config files, then send a SIGHUP so that it rereads
> + *  them.
> + *
> + *  Returns 0 on success, -1 on failure.
> + */
>  static int
> -networkStartRadvd(virNetworkObjPtr network)
> +networkRefreshDhcpDaemon(virNetworkObjPtr network)
>  {
> -    char *pidfile = NULL;
> -    char *radvdpidbase = NULL;
> -    virBuffer configbuf = VIR_BUFFER_INITIALIZER;;
> -    char *configstr = NULL;
> -    char *configfile = NULL;
> -    virCommandPtr cmd = NULL;
>      int ret = -1, ii;
>      virNetworkIpDefPtr ipdef;
> +    dnsmasqContext *dctx = NULL;
>  
> -    network->radvdPid = -1;
> +    /* if there's no running dnsmasq, just start it */
> +    if (network->dnsmasqPid <= 0 || (kill(network->dnsmasqPid, 0) < 0))
> +        return networkStartDhcpDaemon(network);
>  
> -    if (!virFileIsExecutable(RADVD)) {
> -        virReportSystemError(errno,
> -                             _("Cannot find %s - "
> -                               "Possibly the package isn't installed"),
> -                             RADVD);
> -        goto cleanup;
> +    /* Look for first IPv4 address that has dhcp defined. */
> +    /* We support dhcp config on 1 IPv4 interface only. */
> +    for (ii = 0;
> +         (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
> +         ii++) {
> +        if (ipdef->nranges || ipdef->nhosts)
> +            break;
>      }
> +    /* If no IPv4 addresses had dhcp info, pick the first (if there were any). */
> +    if (!ipdef)
> +        ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
>  
> -    if (virFileMakePath(NETWORK_PID_DIR) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot create directory %s"),
> -                             NETWORK_PID_DIR);
> -        goto cleanup;
> -    }
> -    if (virFileMakePath(RADVD_STATE_DIR) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot create directory %s"),
> -                             RADVD_STATE_DIR);
> -        goto cleanup;
> +    if (!ipdef) {
> +        /* no <ip> elements, so nothing to do */
> +        return 0;
>      }
>  
> -    /* construct pidfile name */
> -    if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) {
> -        virReportOOMError();
> +    if (!(dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR)))
>          goto cleanup;
> -    }
> -    if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, radvdpidbase))) {
> -        virReportOOMError();
> +
> +    if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) < 0)
> +       goto cleanup;
> +
> +    if ((ret = dnsmasqSave(dctx)) < 0)
>          goto cleanup;
> +
> +    ret = kill(network->dnsmasqPid, SIGHUP);
> +cleanup:
> +    dnsmasqContextFree(dctx);
> +    return ret;
> +}
> +
> +/* networkRestartDhcpDaemon:
> + *
> + * kill and restart dnsmasq, in order to update any config that is on
> + * the dnsmasq commandline (and any placed in separate config files).
> + *
> + *  Returns 0 on success, -1 on failure.
> + */
> +static int
> +networkRestartDhcpDaemon(virNetworkObjPtr network)
> +{
> +    /* if there is a running dnsmasq, kill it */
> +    if (network->dnsmasqPid > 0) {
> +        networkKillDaemon(network->dnsmasqPid, "dnsmasq",
> +                          network->def->name);
> +        network->dnsmasqPid = -1;
>      }
> +    /* now start dnsmasq if it should be started */
> +    return networkStartDhcpDaemon(network);
> +}
> +#endif /* #if 0 */
> +
> +static int
> +networkRadvdConfContents(virNetworkObjPtr network, char **configstr)
> +{
> +    virBuffer configbuf = VIR_BUFFER_INITIALIZER;;
> +    int ret = -1, ii;
> +    virNetworkIpDefPtr ipdef;
> +    bool v6present = false;
> +
> +    *configstr = NULL;
>  
>      /* create radvd config file appropriate for this network */
>      virBufferAsprintf(&configbuf, "interface %s\n"
> @@ -893,12 +998,15 @@ networkStartRadvd(virNetworkObjPtr network)
>                        "  AdvOtherConfigFlag off;\n"
>                        "\n",
>                        network->def->bridge);
> +
> +    /* add a section for each IPv6 address in the config */
>      for (ii = 0;
>           (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii));
>           ii++) {
>          int prefix;
>          char *netaddr;
>  
> +        v6present = true;
>          prefix = virNetworkIpDefPrefix(ipdef);
>          if (prefix < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -919,30 +1027,118 @@ networkStartRadvd(virNetworkObjPtr network)
>          VIR_FREE(netaddr);
>      }
>  
> -    virBufferAddLit(&configbuf, "};\n");
> +    /* only create the string if we found at least one IPv6 address */
> +    if (v6present) {
> +        virBufferAddLit(&configbuf, "};\n");
>  
> -    if (virBufferError(&configbuf)) {
> -        virReportOOMError();
> -        goto cleanup;
> +        if (virBufferError(&configbuf)) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +        if (!(*configstr = virBufferContentAndReset(&configbuf))) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
>      }
> -    if (!(configstr = virBufferContentAndReset(&configbuf))) {
> -        virReportOOMError();
> +
> +    ret = 0;
> +cleanup:
> +    virBufferFreeAndReset(&configbuf);
> +    return ret;
> +}
> +
> +/* write file and return it's name (which must be freed by caller) */
> +static int
> +networkRadvdConfWrite(virNetworkObjPtr network, char **configFile)
> +{
> +    int ret = -1;
> +    char *configStr = NULL;
> +    char *myConfigFile = NULL;
> +
> +    if (!configFile)
> +        configFile = &myConfigFile;
> +
> +    *configFile = NULL;
> +
> +    if (networkRadvdConfContents(network, &configStr) < 0)
> +        goto cleanup;
> +
> +    if (!configStr) {
> +        ret = 0;
>          goto cleanup;
>      }
>  
>      /* construct the filename */
> -    if (!(configfile = networkRadvdConfigFileName(network->def->name))) {
> +    if (!(*configFile = networkRadvdConfigFileName(network->def->name))) {
>          virReportOOMError();
>          goto cleanup;
>      }
>      /* write the file */
> -    if (virFileWriteStr(configfile, configstr, 0600) < 0) {
> +    if (virFileWriteStr(*configFile, configStr, 0600) < 0) {
>          virReportSystemError(errno,
>                               _("couldn't write radvd config file '%s'"),
> -                             configfile);
> +                             *configFile);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(configStr);
> +    VIR_FREE(myConfigFile);
> +    return ret;
> +}
> +
> +static int
> +networkStartRadvd(virNetworkObjPtr network)
> +{
> +    char *pidfile = NULL;
> +    char *radvdpidbase = NULL;
> +    char *configfile = NULL;
> +    virCommandPtr cmd = NULL;
> +    int ret = -1;
> +
> +    network->radvdPid = -1;
> +
> +    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) {
> +        /* no IPv6 addresses, so we don't need to run radvd */
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    if (!virFileIsExecutable(RADVD)) {
> +        virReportSystemError(errno,
> +                             _("Cannot find %s - "
> +                               "Possibly the package isn't installed"),
> +                             RADVD);
>          goto cleanup;
>      }
>  
> +    if (virFileMakePath(NETWORK_PID_DIR) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot create directory %s"),
> +                             NETWORK_PID_DIR);
> +        goto cleanup;
> +    }
> +    if (virFileMakePath(RADVD_STATE_DIR) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot create directory %s"),
> +                             RADVD_STATE_DIR);
> +        goto cleanup;
> +    }
> +
> +    /* construct pidfile name */
> +    if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +    if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, radvdpidbase))) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (networkRadvdConfWrite(network, &configfile) < 0)
> +        goto cleanup;
> +
>      /* prevent radvd from daemonizing itself with "--debug 1", and use
>       * a dummy pidfile name - virCommand will create the pidfile we
>       * want to use (this is necessary because radvd's internal
> @@ -963,21 +1159,63 @@ networkStartRadvd(virNetworkObjPtr network)
>      if (virCommandRun(cmd, NULL) < 0)
>          goto cleanup;
>  
> -    if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase,
> -                       &network->radvdPid) < 0)
> +    if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, &network->radvdPid) < 0)
>          goto cleanup;
>  
>      ret = 0;
>  cleanup:
>      virCommandFree(cmd);
>      VIR_FREE(configfile);
> -    VIR_FREE(configstr);
> -    virBufferFreeAndReset(&configbuf);
>      VIR_FREE(radvdpidbase);
>      VIR_FREE(pidfile);
>      return ret;
>  }
>  
> +#if 0
> +/* currently unused, so they cause a build error unless we #if them out */
> +static int
> +networkRefreshRadvd(virNetworkObjPtr network)
> +{
> +    /* if there's no running radvd, just start it */
> +    if (network->radvdPid <= 0 || (kill(network->radvdPid, 0) < 0))
> +        return networkStartRadvd(network);
> +
> +    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) {
> +        /* no IPv6 addresses, so we don't need to run radvd */
> +        return 0;
> +    }
> +
> +    if (networkRadvdConfWrite(network, NULL) < 0)
> +        return -1;
> +
> +    return kill(network->radvdPid, SIGHUP);
> +}
> +
> +static int
> +networkRestartRadvd(virNetworkObjPtr network)
> +{
> +    char *radvdpidbase;
> +
> +    /* if there is a running radvd, kill it */
> +    if (network->radvdPid > 0) {
> +        /* essentially ignore errors from the following two functions,
> +         * since there's really no better recovery to be done than to
> +         * just push ahead (and that may be exactly what's needed).
> +         */
> +        if ((networkKillDaemon(network->dnsmasqPid, "radvd",
> +                               network->def->name) >= 0) &&
> +            ((radvdpidbase = networkRadvdPidfileBasename(network->def->name))
> +             != NULL)) {
> +            virPidFileDelete(NETWORK_PID_DIR, radvdpidbase);
> +            VIR_FREE(radvdpidbase);
> +        }
> +        network->radvdPid = -1;
> +    }
> +    /* now start radvd if it should be started */
> +    return networkStartRadvd(network);
> +}
> +#endif /* #if 0 */
> +
>  static int
>  networkAddMasqueradingIptablesRules(struct network_driver *driver,
>                                      virNetworkObjPtr network,

  A bit annoying to add so much #if 0'ed code as we don't check that
code in practice, but okay, ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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