Re: [libvirt] [PATCH 0/3] Add dnsmasq module (v2)
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/