only allowing one connection per IP address

[Trustin Lee]

Hi Frederic,

On Fri, Apr 3, 2009 at 10:54 PM, Frederic Bregier <fredbregier at free.fr> wrote:
> OK, I come back on your comments:
>
>>>  Only channelConnectedAccept and channelClosedWasBlocked (not so happy with this name
>>>  but I couldn't figure something more acceptable rigt now) are still there.
>>
>> I think we can simplify this further.  Can we remove
>> channelConnectedAccept and channelClosedWasBlocked, and just use
>> accept() and the attachment?  We can move the code in
>> channelConnectedAccept() to handleUpstream() and get rid of
>> channelConnectedAccept() and channelClosedWasBlocked().
>
>
> OK, I remove them. I keep :
> -accept
> -handleRefusedChannel
> -isBlocked (see later on)
>
> I also change the FutureChannel handling. Can you check if it seems ok ?

Looks good.  You could use ChannelFutureListener.CLOSE instead of
IpFilteringFutureListener though - we don't need to create code
duplication. ;)

>>> - I only trap by default CONNECTED and CLOSED events.
>>>  However, if a channel was blocked (accept returning false), all events will not
>>>  be passed to the next entry in the pipeline.
>>
>> This is somewhat inconsistent in that there's possibility that
>> channelOpen() and channelBound() can be handed off to the next
>> handlers while channelClosed() will never be handed off.  We should
>> block all events or none.
>
> OK, so I change a bit the logic. All events (except OPEN and BOUND since
> they occur before CONNECT so before the filter) are blocked by default.
> But the user can change it either by overridden isBlocked (in OneIpFilter
> it can be a problem) or by overridden handleUpstream.
> WDYT ?

I'm not sure how this resolves the problem I addressed.  Let's say
that a user put some initialization code in channelOpen().  When a
channel is closed, the user will expect channelClosed() will be
invoked and he or she can destroy resources allocated in
channelOpen().  Every state event must be symmetric - if channelOpen
is fired, channelClosed must be fired in the end.  Same applies for
channelBound/Unbound and channelConnected/Disconnected.  We do have
control over channelConnected/Disconnected, but not over
channelOpen/Closed and channelBound/Unbound.  Perhaps we should
maintain some flags to determine if channelClosed / channelUnbound
needs to be handed off to the next handler?  Pretty tricky..

>> It look OK, but I think it should work fine with IPv6 addresses, too.
>> Could you consider this?
>
> I agreen but why not considering another IpSubnet implementation for IPV6.
> CIDR for instance seems not ok for IPV6 (am I right ?).

CIDR also applies to IPv6.  The only difference is that the prefix
length ranges from 0 to 128.  Therefore, I think IpSubnet should
support both IPv4 and IPv6 address.

>> This needs to be independent from IpSubnet, which means, IpSubnet
>> shouldn't decide its address should be allowed or denied.
>
> I agree, so I introduce a new Interface IpSet which IpSubnet implements.
> Then IpFilterRule is an interface of IpSet plus ALLOW DENY property.
> And then we've got IpSubnetFilterRule which implements this interface
> by extending IpSubnet.
> WDYT ?

Very cool. :)

>> It's getting better and better.  Please keep up the great work!
> I hope so ;-)

It is! :-D

Cheers,

— Trustin Lee, http://gleamynode.net/