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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list