<br><br><div class="gmail_quote">On Sun, Feb 17, 2013 at 6:28 PM, Simon Kelley <span dir="ltr"><<a href="mailto:simon@thekelleys.org.uk" target="_blank">simon@thekelleys.org.uk</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Fantastic! I'm beating my way though my my inbox and just caught up with you. I was intending to put this off to the first thing in 2.67, rather than the last thing in 2.66, but having got this far, I'm not so sure.<br>
</blockquote><div><br></div><div>Wonderful!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
You even wrote the man-page text!<br>
<br>
Queries which arise mainly from a fresh pair of eyes:<br>
<br>
1)  Do we need to support HAVE_OLD_IPSET? I can see more reason for 2.4 kernels that 2.6, but I think most routers are using modern kernels now? <br></blockquote><div><br></div><div>We really do need to support old ipset. Lots of broadcom-based routers run 2.6.22, because that's the kernel version broadcom has released their drivers for. They're extremely widespread, so supporting it is really a must.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">If we do need to support both, I'd like to do both in one binary, just to reduce the potential build confusion.</blockquote>
<div><br></div><div>This shouldn't be _too big_ of a problem. I'm rewriting things now to check utsname on the first invocation of the function, and then route accordingly. The only potential problem is that since we can't rely on the existence of the ipset-netlink headers on older systems, I'll have to import some of those constant values.</div>
<div><br></div><div>Personally I think it's much much much cleaner to have them separate compile time options, but I understand the build confusion issue too.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Am I right that the setsockopt method only supports IPv4? So we can't support just that.</blockquote><div> </div><div>Older ipsets only support IPv4. They're still extremely useful, but it's a limitation.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) Can we get by without adding a dependency on libmnl? Do we want to?<br>
   even if we don't, we can replace the code which starts<br></blockquote><div><br></div><div>The ipset userland tool uses libmnl, so it's already on the user's system if they're using ipset. However, this isn't the case if they're using old ipset, so it might be best to move away from libmnl. I'm investigating that right now.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
mnl = mnl_socket_open(NETLINK_<u></u>NETFILTER);<br>
<br>
and ends<br>
<br>
mnl_socket_close(mnl);<br>
<br>
with something which uses the already-existing netlink socket created in src/netlink.c, and avoid the overhead of making a new netlink socket on each iteration?  Worry about process_reply() being called from the TCP-request code-path, from a forked process. I think the netlink socket inherited by the forked process will still be OK.<br>
</blockquote><div><br></div><div>If we do stick with libmnl, implementing this will be no problem.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
3)<br>
(af == AF_INET ? sizeof(ipaddr->addr.addr4) : sizeof(ipaddr->addr.addr4)),<br>
<br>
second "addr4" should be "addr6"?<br></blockquote><div><br></div><div>Good catch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
4) The code should compile if HAVE_IPV6 is not set even if the headers don't define AF_INET6<br></blockquote><div><br></div><div>Roger that.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
5) Compile-time options need to be added to the prepocessor stuff defining the compile_opts string in src/config.c<br></blockquote><div><br></div><div>I actually added this after the initial email in the git repository -- <a href="http://git.zx2c4.com/dnsmasq-ipset/">http://git.zx2c4.com/dnsmasq-ipset/</a> .</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
<br>
Simon.<br>
<br>
______________________________<u></u>_________________<br>
Dnsmasq-discuss mailing list<br>
<a href="mailto:Dnsmasq-discuss@lists.thekelleys.org.uk" target="_blank">Dnsmasq-discuss@lists.<u></u>thekelleys.org.uk</a><br>
<a href="http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss" target="_blank">http://lists.thekelleys.org.<u></u>uk/mailman/listinfo/dnsmasq-<u></u>discuss</a><br>
</blockquote></div><br>