Message-ID: <f4845fc0808211501y188b0ae9ie7aacb2f52c3dcd8@mail.gmail.com>
Date:	Fri, 22 Aug 2008 00:01:19 +0200
From:	"Julius Volz" <juliusv@...gle.com>
To:	"Simon Horman" <horms@...ge.net.au>
Cc:	netdev@...r.kernel.org, lvs-devel@...r.kernel.org, kaber@...sh.net,
	vbusam@...gle.com, "Sven Wegener" <sven.wegener@...aler.net>
Subject: Re: [PATCH RFC 00/24] IPVS: Add first IPv6 support to IPVS

On Thu, Aug 21, 2008 at 11:29 PM, Simon Horman <horms@...ge.net.au> wrote:
> On Thu, Aug 21, 2008 at 11:59:44AM +0200, Julius Volz wrote:
>> There shouldn't be any fundamental restrictions, it's just a piece of
>> the puzzle that I could easily leave out of the picture for now.
>>
>> I haven't studied the sync daemon closely yet, but one thing I was
>> briefly wondering about was whether we should just blow up the
>> addresses in struct ip_vs_sync_conn to be of type union nf_inet_addr
>> (probably not acceptable, wasting too much bandwidth for v4 entries)
>> or how to send differently sized entries based on the IP version in a
>> clean way. But it sounds like you'd want to redesign a lot of that
>> anyways? I'm glad to help with anything, I just don't know this code
>> as well as you or Sven, but I'll study it more. Maybe you can share
>> some ideas on the extensibility you want to see?
>
> What I was thinking is that any change

Hm, I think you forgot to finish this sentence ;)

> Here are a few thoughts I have had in my breif overview of the code so far
> - mainly just simple style things. If any of my comments are obviously
> stupid, please just say so as I haven't got to the end of the series yet
> and I'm sure some of my questions are answered in the code.

Thanks for looking at this, no problem. I know it's a bit much to
digest, but I think there is no smaller part that I could post that is
useful in itself already.

> [PATCH RFC 01/24] IPVS: Add genetlink interface definitions to ip_vs.h
> [PATCH RFC 02/24] IPVS: Add genetlink interface implementation
>
> * Already in lvs-2.6. Are there any changes?

No, those are just exactly what I sent you before. I included them
because I was basing it on net-2.6.

Btw., David just announced that he opened net-next-2.6, so perhaps he
could pull the changes from your tree into that now?

> [PATCH RFC 02/24] IPVS: Add genetlink interface implementation
>
> * What is IP_VS = IPV6 ?

(IPV6 = y || IP_VS = IPV6) means that this option will only be visible
if either CONFIG_IPV6 is set to Y or if both CONFIG_IPV6 and
CONFIG_IP_VS are modules (if both are off, the whole submenu will be
hidden). So CONFIG_IPV6 has to be set at least as high as CONFIG_IP_VS
for this option to be visible.

> [PATCH RFC 04/24] IPVS: Change IPVS data structures to support IPv6 addresses
>
> * Indentation of af in struct ip_vs_conn seems inconsistent with other
>  elements.

Yeah, it's broken like this in the original and I kept it like that so
that things would still line up (because I didn't want to add noise by
touching the neighboring lines just for whitespace fixes). I could
just send a small whitespace fix to you before.

> [PATCH RFC 05/24] IPVS: Add general v4/v6 helper functions / data structures
>
> * Use of p ? a : b construct in ip_vs_addr_equal() seems a bit aquard.
>  How about
>
> static inline int ip_vs_addr_equal(int af, const union nf_inet_addr *a,
>                                   const union nf_inet_addr *b)
> {
> #ifdef CONFIG_IP_VS_IPV6
>        if (af == AF_INET)
>                return ipv6_addr_equal(&a->in6, &b->in6);
> #endif
>        return a->ip == b->ip;
> }

Right, that is nicer! With AF_INET6 in that if-condition, of course.

Julius

-- 
Google Switzerland GmbH
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists