virNetworkAssignDef was allocating a new network object, initing and grabbing its lock, then potentially freeing it without unlocking or destroying the lock. In practice 1) this will probably never happen, and 2) even if it did, the lock implementation used on most (all?) platforms doesn't actually hold any resources for an initialized or held lock, but it still bothered me, so I moved the realloc that could lead to this bad situation earlier in the function, and now the mutex isn't inited or locked until we are assured of complete success. --- src/conf/network_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d916427..88e1492 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -245,6 +245,11 @@ virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, return network; } + if (VIR_REALLOC_N(nets->objs, nets->count + 1) < 0) { + virReportOOMError(); + return NULL; + } + if (VIR_ALLOC(network) < 0) { virReportOOMError(); return NULL; @@ -258,12 +263,6 @@ virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjLock(network); network->def = def; - if (VIR_REALLOC_N(nets->objs, nets->count + 1) < 0) { - virReportOOMError(); - VIR_FREE(network); - return NULL; - } - nets->objs[nets->count] = network; nets->count++; -- 1.7.11.4

This is very short, because almost everything is autogenerated. All that's needed are: * src/remote/remote_driver.c: add pointer to autogenerated remoteNetworkUpdate to the function table for the remote network driver. * src/remote/remote_protocol.x: add the "args" struct and add one more item to the remote_procedure enum for this function. * src/remote_protocol-struct: update to match remote_protocol.x --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 11 ++++++++++- src/remote_protocol-structs | 8 ++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8f3895d..2a30096 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6075,6 +6075,7 @@ static virNetworkDriver network_driver = { .networkCreateXML = remoteNetworkCreateXML, /* 0.3.0 */ .networkDefineXML = remoteNetworkDefineXML, /* 0.3.0 */ .networkUndefine = remoteNetworkUndefine, /* 0.3.0 */ + .networkUpdate = remoteNetworkUpdate, /* 0.10.2 */ .networkCreate = remoteNetworkCreate, /* 0.3.0 */ .networkDestroy = remoteNetworkDestroy, /* 0.3.0 */ .networkGetXMLDesc = remoteNetworkGetXMLDesc, /* 0.3.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 6201ff7..2db09f2 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1328,6 +1328,14 @@ struct remote_network_undefine_args { remote_nonnull_network net; }; +struct remote_network_update_args { + remote_nonnull_network net; + unsigned int section; + int parentIndex; + remote_nonnull_string xml; + unsigned int flags; +}; + struct remote_network_create_args { remote_nonnull_network net; }; @@ -2988,7 +2996,8 @@ enum remote_procedure { REMOTE_PROC_CONNECT_LIST_ALL_NWFILTERS = 286, /* skipgen skipgen priority:high */ REMOTE_PROC_CONNECT_LIST_ALL_SECRETS = 287, /* skipgen skipgen priority:high */ REMOTE_PROC_NODE_SET_MEMORY_PARAMETERS = 288, /* autogen autogen */ - REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289 /* skipgen skipgen */ + REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, /* skipgen skipgen */ + REMOTE_PROC_NETWORK_UPDATE = 290 /* autogen autogen priority:high */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 52ccf80..a84ed86 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -969,6 +969,13 @@ struct remote_network_define_xml_ret { struct remote_network_undefine_args { remote_nonnull_network net; }; +struct remote_network_update_args { + remote_nonnull_network net; + u_int section; + int index; + remote_nonnull_string xml; + u_int flags; +}; struct remote_network_create_args { remote_nonnull_network net; }; @@ -2397,4 +2404,5 @@ enum remote_procedure { REMOTE_PROC_CONNECT_LIST_ALL_SECRETS = 287, REMOTE_PROC_NODE_SET_MEMORY_PARAMETERS = 288, REMOTE_PROC_NODE_GET_MEMORY_PARAMETERS = 289, + REMOTE_PROC_NETWORK_UPDATE = 290, }; -- 1.7.11.4

virNetworkObjUpdate takes care of all virNetworkUpdate-related changes to the data stored in the in-memory virNetworkObj list. It should be called by network drivers that use this in-memory list. virNetworkObjUpdate *does not* take care of updating any disk-based copies of the config, nor does it perform any other operations necessary to have the new config data take effect (e.g. it won't re-write dnsmasq host files, nor will it send a SIGHUP to dnsmasq) - those things should all be taken care of in the network driver function that calls virNetworkObjUpdate (assuming that it returns success). --- src/conf/network_conf.c | 262 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 7 ++ src/libvirt_private.syms | 1 + 3 files changed, 270 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a48eb9e..8f65c84 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2251,6 +2251,268 @@ void virNetworkSetBridgeMacAddr(virNetworkDefPtr def) } } +/* NetworkObj backend of the virNetworkUpdate API */ + +static void +virNetworkDefUpdateNoSupport(virNetworkDefPtr def, const char *section) +{ + virReportError(VIR_ERR_NO_SUPPORT, + _("can't update '%s' section of network '%s'"), + section, def->name); +} + +static int +virNetworkDefUpdateBridge(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "bridge"); + return -1; +} + +static int +virNetworkDefUpdateDomain(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "domain"); + return -1; +} + +static int +virNetworkDefUpdateIP(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "ip"); + return -1; +} + +static int +virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "ip dhcp host"); + return -1; +} + +static int +virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "ip dhcp range"); + return -1; +} + +static int +virNetworkDefUpdateForward(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "forward"); + return -1; +} + +static int +virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "forward interface"); + return -1; +} + +static int +virNetworkDefUpdateForwardPF(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "forward pf"); + return -1; +} + +static int +virNetworkDefUpdatePortgroup(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "portgroup"); + return -1; +} + +static int +virNetworkDefUpdateDNSHost(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "dns host"); + return -1; +} + +static int +virNetworkDefUpdateDNSTxt(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "dns txt"); + return -1; +} + +static int +virNetworkDefUpdateDNSSrv(virNetworkDefPtr def, + int parentIndex ATTRIBUTE_UNUSED, + const char *xml ATTRIBUTE_UNUSED, + /* virNetworkUpdateFlags */ + unsigned int fflags ATTRIBUTE_UNUSED) +{ + virNetworkDefUpdateNoSupport(def, "dns txt"); + return -1; +} + +static int +virNetworkDefUpdateSection(virNetworkDefPtr def, + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags) /* virNetworkUpdateFlags */ +{ + int ret = -1; + + switch (section) { + case VIR_NETWORK_SECTION_BRIDGE: + ret = virNetworkDefUpdateBridge(def, parentIndex, xml, flags); + break; + + case VIR_NETWORK_SECTION_DOMAIN: + ret = virNetworkDefUpdateDomain(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_IP: + ret = virNetworkDefUpdateIP(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_IP_DHCP_HOST: + ret = virNetworkDefUpdateIPDHCPHost(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_IP_DHCP_RANGE: + ret = virNetworkDefUpdateIPDHCPRange(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_FORWARD: + ret = virNetworkDefUpdateForward(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_FORWARD_INTERFACE: + ret = virNetworkDefUpdateForwardInterface(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_FORWARD_PF: + ret = virNetworkDefUpdateForwardPF(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_PORTGROUP: + ret = virNetworkDefUpdatePortgroup(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_DNS_HOST: + ret = virNetworkDefUpdateDNSHost(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_DNS_TXT: + ret = virNetworkDefUpdateDNSTxt(def, parentIndex, xml, flags); + break; + case VIR_NETWORK_SECTION_DNS_SRV: + ret = virNetworkDefUpdateDNSSrv(def, parentIndex, xml, flags); + break; + default: + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("can't update unrecognized section of network")); + break; + } + + return ret; +} + +/* + * virNetworkObjUpdate: + * + * Apply the supplied update to the given virNetworkObj. Except for + * @network pointing to an actual network object rather than the + * opaque virNetworkPtr, parameters are identical to the public API + * virNetworkUpdate. + * + * The original virNetworkDefs are copied, and all modifications made + * to these copies. The originals are replaced with the copies only + * after success has been guaranteed. + * + * Returns: -1 on error, 0 on success. + */ +int +virNetworkObjUpdate(virNetworkObjPtr network, + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags) /* virNetworkUpdateFlags */ +{ + int ret = -1; + virNetworkDefPtr def = NULL; + + /* normalize config data, and check for common invalid requests. */ + if (virNetworkConfigChangeSetup(network, flags) < 0) + goto cleanup; + + if (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE) { + /* work on a copy of the def */ + if (!(def = virNetworkDefCopy(network->def, 0))) + goto cleanup; + if (virNetworkDefUpdateSection(def, section, + parentIndex, xml, flags) < 0) { + goto cleanup; + } + /* successfully modified copy, now replace original */ + virNetworkDefFree(network->def); + network->def = def; + def = NULL; + } + + if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + /* work on a copy of the def */ + if (!(def = virNetworkDefCopy(virNetworkObjGetPersistentDef(network), + VIR_NETWORK_XML_INACTIVE))) { + goto cleanup; + } + if (virNetworkDefUpdateSection(def, section, + parentIndex, xml, flags) < 0) { + goto cleanup; + } + /* successfully modified copy, now replace original */ + if (virNetworkObjReplacePersistentDef(network, def) < 0) + goto cleanup; + def = NULL; + } + + ret = 0; +cleanup: + virNetworkDefFree(def); + return ret; +} + /* * virNetworkObjIsDuplicate: * @doms : virNetworkObjListPtr to search diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 0d37a8b..3d12f58 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -326,6 +326,13 @@ int virNetworkSetBridgeName(const virNetworkObjListPtr nets, void virNetworkSetBridgeMacAddr(virNetworkDefPtr def); +int +virNetworkObjUpdate(virNetworkObjPtr obj, + unsigned int section, /* virNetworkUpdateSection */ + int parentIndex, + const char *xml, + unsigned int flags); /* virNetworkUpdateFlags */ + int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, virNetworkDefPtr def, unsigned int check_active); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33763fc..29511f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -853,6 +853,7 @@ virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; virNetworkObjUnlock; +virNetworkObjUpdate; virNetworkRemoveInactive; virNetworkSaveConfig; virNetworkSaveStatus; -- 1.7.11.4

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, -- 1.7.11.4

Call the network_conf function that modifies the live/persistent/both config, then refresh/restart dnsmasq/radvd if necessary, and finally save the config in the proper place(s). This patch also needed to uncomment a few utility functions that were added inside #if 0 in the previous commit (to avoid compiler errors due to unreferenced static functions). --- src/network/bridge_driver.c | 123 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 115 insertions(+), 8 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5f7f31e..37447e8 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -468,8 +468,6 @@ 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. @@ -528,7 +526,6 @@ networkKillDaemon(pid_t pid, const char *daemonName, const char *networkName) cleanup: return ret; } -#endif /* #if 0 */ static int networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, @@ -908,8 +905,6 @@ 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. @@ -978,7 +973,6 @@ networkRestartDhcpDaemon(virNetworkObjPtr network) /* now start dnsmasq if it should be started */ return networkStartDhcpDaemon(network); } -#endif /* #if 0 */ static int networkRadvdConfContents(virNetworkObjPtr network, char **configstr) @@ -1171,8 +1165,6 @@ cleanup: return ret; } -#if 0 -/* currently unused, so they cause a build error unless we #if them out */ static int networkRefreshRadvd(virNetworkObjPtr network) { @@ -1191,6 +1183,8 @@ networkRefreshRadvd(virNetworkObjPtr network) return kill(network->radvdPid, SIGHUP); } +#if 0 +/* currently unused, so it causes a build error unless we #if it out */ static int networkRestartRadvd(virNetworkObjPtr network) { @@ -2830,6 +2824,118 @@ cleanup: return ret; } +static int +networkUpdate(virNetworkPtr net, + unsigned int section, + int parentIndex, + const char *xml, + unsigned int flags) +{ + struct network_driver *driver = net->conn->networkPrivateData; + virNetworkObjPtr network = NULL; + int isActive, ret = -1; + + virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE | + VIR_NETWORK_UPDATE_AFFECT_CONFIG | + VIR_NETWORK_UPDATE_EXISTING | + VIR_NETWORK_UPDATE_DELETE | + VIR_NETWORK_UPDATE_ADD_LAST | + VIR_NETWORK_UPDATE_ADD_FIRST, + -1); + + networkDriverLock(driver); + + network = virNetworkFindByUUID(&driver->networks, net->uuid); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + "%s", _("no network with matching uuid")); + goto cleanup; + } + + /* VIR_NETWORK_UPDATE_AFFECT_CURRENT means "change LIVE if network + * is active, else change CONFIG + */ + isActive = virNetworkObjIsActive(network); + if ((flags & (VIR_NETWORK_UPDATE_AFFECT_LIVE + | VIR_NETWORK_UPDATE_AFFECT_CONFIG)) == + VIR_NETWORK_UPDATE_AFFECT_CURRENT) { + if (isActive) + flags |= VIR_NETWORK_UPDATE_AFFECT_LIVE; + else + flags |= VIR_NETWORK_UPDATE_AFFECT_CONFIG; + } + + /* update the network config in memory/on disk */ + if (virNetworkObjUpdate(network, section, parentIndex, xml, flags) < 0) + goto cleanup; + + if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { + /* save updated persistent config to disk */ + if (virNetworkSaveConfig(driver->networkConfigDir, + virNetworkObjGetPersistentDef(network)) < 0) { + goto cleanup; + } + } + + if (isActive && (flags & VIR_NETWORK_UPDATE_AFFECT_LIVE)) { + /* rewrite dnsmasq host files, restart dnsmasq, update iptables + * rules, etc, according to which section was modified. Note that + * some sections require multiple actions, so a single switch + * statement is inadequate. + */ + if (section == VIR_NETWORK_SECTION_BRIDGE || + section == VIR_NETWORK_SECTION_DOMAIN || + section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_IP_DHCP_RANGE) { + /* these sections all change things on the dnsmasq commandline, + * so we need to kill and restart dnsmasq. + */ + if (networkRestartDhcpDaemon(network) < 0) + goto cleanup; + + } else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST || + section == VIR_NETWORK_SECTION_DNS_HOST || + section == VIR_NETWORK_SECTION_DNS_TXT || + section == VIR_NETWORK_SECTION_DNS_SRV) { + /* these sections only change things in config files, so we + * can just update the config files and send SIGHUP to + * dnsmasq. + */ + if (networkRefreshDhcpDaemon(network) < 0) + goto cleanup; + + } + + if (section == VIR_NETWORK_SECTION_IP) { + /* only a change in IP addresses will affect radvd, and all of radvd's + * config is stored in the conf file which will be re-read with a SIGHUP. + */ + if (networkRefreshRadvd(network) < 0) + goto cleanup; + } + + if (section == VIR_NETWORK_SECTION_IP || + section == VIR_NETWORK_SECTION_FORWARD || + section == VIR_NETWORK_SECTION_FORWARD_INTERFACE) { + /* these could affect the iptables rules */ + networkRemoveIptablesRules(driver, network); + if (networkAddIptablesRules(driver, network) < 0) + goto cleanup; + + } + + /* save current network state to disk */ + if ((ret = virNetworkSaveStatus(NETWORK_STATE_DIR, network)) < 0) + goto cleanup; + } + ret = 0; +cleanup: + if (network) + virNetworkObjUnlock(network); + networkDriverUnlock(driver); + return ret; +} + static int networkStart(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; @@ -3057,6 +3163,7 @@ static virNetworkDriver networkDriver = { .networkCreateXML = networkCreate, /* 0.2.0 */ .networkDefineXML = networkDefine, /* 0.2.0 */ .networkUndefine = networkUndefine, /* 0.2.0 */ + .networkUpdate = networkUpdate, /* 0.10.2 */ .networkCreate = networkStart, /* 0.2.0 */ .networkDestroy = networkDestroy, /* 0.2.0 */ .networkGetXMLDesc = networkGetXMLDesc, /* 0.2.0 */ -- 1.7.11.4

A user on IRC had accidentally killed all of his libvirt-started dnsmasq instances (due to a buggy dnsmasq service script in Fedora 16), and had hoped that libvirtd would notice this on restart and reload all the dnsmasq daemons (as it does with iptables rules). Unfortunately this was not the case - as long as the network object had a pid registered for dnsmasq and/or radvd, it assumed that the processes were running. This patch takes advantage of the new utility functions in bridge_driver.c to do a "refresh" of all radvd and dnsmasq processes started by libvirt each time libvirtd is restarted - this function attempts to do a SIGHUP of each existing process, and if that fails, it restarts the process, rebuilding all the associated config files and commandline parameters in the process. This normally has no effect, but will be useful in solving the occasional "odd situation" without needing to take the drastic step of destroying/re-starting the network. --- src/network/bridge_driver.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 37447e8..84a8510 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -117,6 +117,7 @@ static int networkShutdownNetworkExternal(struct network_driver *driver, virNetworkObjPtr network); static void networkReloadIptablesRules(struct network_driver *driver); +static void networkRefreshDaemons(struct network_driver *driver); static struct network_driver *driverState = NULL; @@ -344,6 +345,7 @@ networkStartup(int privileged) { networkFindActiveConfigs(driverState); networkReloadIptablesRules(driverState); + networkRefreshDaemons(driverState); networkAutostartConfigs(driverState); networkDriverUnlock(driverState); @@ -404,6 +406,7 @@ networkReload(void) { driverState->networkConfigDir, driverState->networkAutostartDir); networkReloadIptablesRules(driverState); + networkRefreshDaemons(driverState); networkAutostartConfigs(driverState); networkDriverUnlock(driverState); return 0; @@ -1210,6 +1213,37 @@ networkRestartRadvd(virNetworkObjPtr network) } #endif /* #if 0 */ +/* SIGHUP/restart any dnsmasq or radvd daemons. + * This should be called when libvirtd is restarted. + */ +static void +networkRefreshDaemons(struct network_driver *driver) +{ + unsigned int i; + + VIR_INFO("Refreshing network daemons"); + + for (i = 0 ; i < driver->networks.count ; i++) { + virNetworkObjPtr network = driver->networks.objs[i]; + + virNetworkObjLock(network); + if (virNetworkObjIsActive(network) && + ((network->def->forwardType == VIR_NETWORK_FORWARD_NONE) || + (network->def->forwardType == VIR_NETWORK_FORWARD_NAT) || + (network->def->forwardType == VIR_NETWORK_FORWARD_ROUTE))) { + /* Only the three L3 network types that are configured by + * libvirt will have a dnsmasq or radvd daemon associated + * with them. Here we send a SIGHUP to an existing + * dnsmasq and/or radvd, or restart them if they've + * disappeared. + */ + networkRefreshDhcpDaemon(network); + networkRefreshRadvd(network); + } + virNetworkObjUnlock(network); + } +} + static int networkAddMasqueradingIptablesRules(struct network_driver *driver, virNetworkObjPtr network, -- 1.7.11.4