[quagga-dev 6620] Re: fd leak in bgpd

On Tue, Jun 02, 2009 at 04:32:31PM +0100, paul <at> jakma.org wrote:

> Hmm, or we stick with your original proposal but remove the problematic 
> from bgp_stop (or split up bgp_stop). All transitions from Established have 
> to go through Clearing:
> 
> 
> Established -> (shutdown) -> Clearing -> (stop) -> Idle

Ahh, yes. I like this a lot better.

> The stop code path == shutdown + flush. shutdown should be
> idem-potent.

Since bgp_stop is idempotent today apart from the event flush, you could
instead change bgp_stop to not call event flush in Established and then
add an entry function to Idle that just flushes the queue from Clearing.
That would be clearer than the equally viable alternative of just
calling bgp_stop again at that point.

This has the advantage of not requiring devs to know the difference
between stop & shutdown, merely requiring a single comment around the
'duplicated' call to bgp_stop in the state table.

> Transitions into Idle directly can call either stop or shutdown. Calling 
> just shutdown might result in further events in Idle besides stop, but can 
> be ignored - unneeded but harmless.

Seems like a couple of comments to this effect might be useful,
specifically to the tune of "Use bgp_stop except when in Established". See
above wrt 'developer proofing'.

> That can happen with transition into Idle via bgp_stop_with_error

bgp_stop_with_error could just call bgp_stop if you're in Established.

> The off-the-cuff, untested, not-ready-for-integration patch is attached, 
> what do you think?

I dunno what you've got in your local git, but the state table changes
wouldn't apply cleanly to 0.99.12 which should be the same as
quagga.git for this. *shrug*

I'll be testing your patch over the next few days to see if anything
comes up. I definitely think this is a cleaner implementation than
my solution.

Thanks,
Steve