On Wed, Jan 14, 2009 at 12:44:50PM +0000, Daniel P. Berrange wrote:
On Wed, Jan 14, 2009 at 08:55:46AM +0000, Mark McLoughlin wrote:
On Tue, 2009-01-13 at 20:49 +0000, Daniel P. Berrange wrote:
Currently when we shutdown the virtual networks are all shutdown too. This is less than useful if we're letting guest VMs hang around post shutdown of libvirtd, because it means we're tearing their network connection out from under them. This patch fixes that allowing networks to survive restarts, and be re-detected next time around.
When starting a virtual network we write the live config into
/var/lib/libvirt/network/$NAME.xml
This is because the bridge device name is potentially auto-generated and we need to keep track of that
We change dnsmasq args to include an explicit pidfile location
/var/run/libvirt/network/$NAME.pid
and also tell it to put itself into the background - ie daemonize. This is because we want dnsmasq to survive the daemon.
Now, when libvirtd starts up it
- Looks for the live config, and if found loads it. - Calls a new method brHasBridge() to see if its desired bridge actually exists (and thus whether the network is running). If it exists,the network is marked active - If DHCP is configured, then reads the dnsmasq PIDfile, and sends kill($PID, 0) to check if the process is actually alive
In addition I cleanup the network code to remove the configFile and autostartLink fields in virNetworkObjPtr, so it matches virDomaiObjPtr usage.
With all this applied you can now restart the daemon, and virbr0 is left happily running.
It all sounds and looks good to me. Some comments below, but nothing major.
Here's an updated patch with all your comments incorporated.
Sounds a real user improvement, patch looks fine but I'm concerned about this:
+ /* Finally try and read dnsmasq pid if any DHCP ranges are set */ + if (obj->def->nranges && + virFileReadPid(NETWORK_PID_DIR, obj->def->name, + &obj->dnsmasqPid) == 0) { + + /* Check its still alive */ + if (kill(obj->dnsmasqPid, 0) != 0) + obj->dnsmasqPid = -1; + + /* XXX ideally we'd check this was actually + * the dnsmasq process, not a stale pid file + * with someone else's process. But how ? + */
OS specific but check in configure about the location of dnsmasq export it as DNSMASQ_PATH, then compare to file pointed by /proc/pid/exe #ifdef __linux__ char *pidpath; virAsprintf(&pidpath, "/proc/%d/exe", obj->dnsmasqPid); if (virFileLinkPointsTo(pidpath, DNSMASQ_PATH) == 0)) obj->dnsmasqPid = -1; VIR_FREE(pidpath); #endif I'm sure one can find a Solaris equivalent. Patch looks fine to me, 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/