On Sat, Apr 24, 2010 at 04:46:33AM +0900, Satoru SATOH wrote:
On Fri, Apr 23, 2010 at 09:33:20AM +0200, Daniel Veillard wrote:
looking at the patches, those looks fine to me, I may have just a couple of cosmetic comments, but I'm wondering if they should be postponed after 0.8.1, or if it's fine to push them in now. On one hand I would prefer to limit the number of actual features in 0.8.1, but on the other hand you have already submitted this many time so I wonder what you think. When you say "prototype implementation" how confident are you about this code ?
So what do you think ?
As far as I tested, it works as expected and not aware of any critical issues. So if you're ok, I want to get it merge in.
Okay, after Jim's thorough review and doing a bit of testing I commited your patch. I changed only a couple of things: - the dead store pointed out by Jim in his last review - the DNSMASQ_STATE_DIR, the host files are managed by libvirt, and even if they are used by dnsmasq, they really need to be stored in a directory owned and managed by libvirt. So I fixed DNSMASQ_STATE_DIR, to be LOCAL_STATE_DIR "/lib/libvirt/network", in practice the same as NETWORK_STATE_DIR. I think its fine because the naming of the files can't clash since they use different suffixes. I tested this on my own boxes and this seems to work as expected, there is just one small bit, assuming one stops a network like in, virsh net-destroy default, the file /var/lib/libvirt/network/default.hostsfile remains while it should really be removed. But it's not a big deal at this point, but I would like to get a fix for this :-) thanks ! 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/