On Wed, 21 Dec 2016 15:44:49 +0100 Willy Tarreau <[email protected]> wrote:
> Hi guys, > > so I've looked a little bit at this and can propose something different. > > On Wed, Dec 14, 2016 at 02:59:50PM +0000, David CARLIER wrote: > > Hi, > > > > On 14 December 2016 at 14:48, <[email protected]> wrote: > > > Hi, thanks for the patch. > > > > > > Maybe it is more efficient to simply add a "#define _KERNEL", or the > > > following code: > > > > > > #if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) > > > #define _KERNEL > > > #endif > > > > > > > It is easy (and temptating) but not a good practice ... in my opinion :-) > > I find this ugly as well and would prefer to avoid it. The main reason > it was done is to ensure userspace does *not* use it so we can't claim > it to be an accident. > > > > I'm not sure that src_hlua.c was the good file for adding these > > > kinds of defines or includes. Even if these defines are used only in > > > this file, I suppose that it should be used in other file (in the > > > future). > > > > > > > I was not sure myself, I hesitated to put it in one of the headers ... > > let me know what you all think. > > I'm seeing that in pattern.c we have the same but the entry is called > "s6_addr", and nobody has ever complained that pattern.c did not build > since 1.5 or so. Thus in my opinion the correct fix would be to replace > all those s6_addr32[] with s6_addr[], eg (for 32-bit use) : > > (*(uint32_t*)&smp->data.u.ipv6.s6_addr[0] > > Thus it would give this : > > > diff --git a/src/hlua_fcn.c b/src/hlua_fcn.c > index 5ac533a..97a4d51 100644 > --- a/src/hlua_fcn.c > +++ b/src/hlua_fcn.c > @@ -1016,14 +1016,16 @@ int hlua_match_addr(lua_State *L) > return 1; > } > } else { > - if (((addr1->addr.v6.ip.s6_addr32[0] & > addr2->addr.v6.mask.s6_addr32[0]) == > - (addr2->addr.v6.ip.s6_addr32[0] & > addr1->addr.v6.mask.s6_addr32[0])) && > - ((addr1->addr.v6.ip.s6_addr32[1] & > addr2->addr.v6.mask.s6_addr32[1]) == > - (addr2->addr.v6.ip.s6_addr32[1] & > addr1->addr.v6.mask.s6_addr32[1])) && > - ((addr1->addr.v6.ip.s6_addr32[2] & > addr2->addr.v6.mask.s6_addr32[2]) == > - (addr2->addr.v6.ip.s6_addr32[2] & > addr1->addr.v6.mask.s6_addr32[2])) && > - ((addr1->addr.v6.ip.s6_addr32[3] & > addr2->addr.v6.mask.s6_addr32[3]) == > - (addr2->addr.v6.ip.s6_addr32[3] & > addr1->addr.v6.mask.s6_addr32[3]))) { > + int i; > + > + for (i = 0; i < 16; i += 4) { > + if ((*(uint32_t *)&addr1->addr.v6.ip.s6_addr[i] & > + *(uint32_t *)&addr2->addr.v6.mask.s6_addr[i]) != > + (*(uint32_t *)&addr2->addr.v6.ip.s6_addr[i] & > + *(uint32_t *)&addr1->addr.v6.mask.s6_addr[i])) > + break; > + } > + if (i == 16) { > lua_pushboolean(L, 1); > return 1; > } Hi, thanks Willy for the idea. I will write a patch ASAP, but. why a 32bits cast and not a 64 bit cast ? Thierry > Also I'm a bot bothered by something I don't understand in the current > code : we have two masks and we apply one address to the other address' > mask. In fact I don't understand the intended operation, even after > reading the doc. The doc says : > > Match two networks. For example "127.0.0.1/32" matchs "127.0.0.0/8". > The order of network is not important. > > And for me this is a perfect example where it's wrong because that doesn't > make sense. None of this network "match" the other one. The first one is > included in the second one and not the opposite. I suspect we have > something deeply wrong there but with a small impact which requires a fix. > > Can someone enlighten me ? > > Willy
