[quagga-dev 11414] Re: quagga/dmvpn work

Timo Teras timo.teras at iki.fi
Thu Aug 14 09:28:18 BST 2014


On Tue, 12 Aug 2014 18:54:30 +0200
David Lamparter <equinox at diac24.net> wrote:

> On Tue, May 20, 2014 at 05:20:16PM +0300, Timo Teras wrote:
> > 1. As optimization, implement API that uses SOCK_CLOEXEC and
> > SOCK_NONBLOCK with socket() and accept4() where possible. This is
> > mostly an optimization. sockopt_socket() or similar? And define some
> > libzebra flags that map to SOCK_* ?
> 
> While nice to have, I don't think these provide huge benefits, but if
> you have/want them I see no problem either.
> 
> Defining additional names for OS constants should be avoided since any
> reader of the code will have to learn them again...  better to stick
> with the OS ones - unless there are really weird differences between
> OSes.  Haven't checked if this applies to the SOCK_* values
> (FD_CLOEXEC vs. O_CLOEXEC vs.  SOCK_CLOEXEC?)

Right. I'll postpone this until later if really needed. In quagga case
the it's mostly a minor optimization. In threaded programs it is
required to make everything fork() safe. Otherwise there could be
fd-leaks on long running fork+execve.

> > 2. Move the netlink helpers from zebra to lib, and clean them up. I
> > remember there was discussion on using some existing library instead
> > (libnl, libmnl). But I'd rather not do that for the reasons that:
> >   a) introduction of additional dependency
> 
> This does _not_ count IMHO.  Reimplementing things means
> re-maintaining things.  And even embedded routers have enough flash
> these days.

Hmm... Ok. I could take a look at again if the issues have been
addressed in newer versions of the libraries. Though, the needed
functionality is small. And the binary format was designed such that
macroes/inline functions could be used. Which makes the whole thing
faster. Generally the inline function results in smaller code than what
the function call would be.

> >   b) duplication of socket handling code that already exists in
> > quagga c) they are bloated: even libmnl implements two variants of
> > helpers (struct nlattr and struct nlmsghdr). single set is enough.
> 
> I haven't looked at it, but if you have, and you say that the existing
> libraries suck, then I'll stick with your evaluation.  Netlink
> wrapping isn't particularly large, so if the existing libs aren't
> nice for us I'd say write them anew...

We basically need only the message formatting and parsing ones only.
Which are 90-95% inline stuff. With few functions that make sense as
functions. And then on top of that is the protocol specific
enumerations and structures that come from linux kernel headers
directly in all cases.

> >   d) we would need additional wrapping to integrate it to quagga
> >      mainloop
> 
> ... I'm still hoping that at some point we can delete the zebra
> mainloop and use some library instead, but for now that's just
> inviting trouble...

I started on working my code and noticed some awkward issues with the
current implementation. Mostly copying around 'struct thread', and not
being able to shutdown cleanly. The way the current code exists from
signal handler is not elegant.

The whole 'thread' naming is also confusing. Not sure how it could be
easily replaced though.

I've been successfully using libev for quite a time now. Ideally I'd
like to use some sort of local continuations that make the code more
readable. But it really depends on the task how it should be.

Given how the current quagga mainloop works like, I was almost
reconsidering writing my code as standalone (with optional zebra
integration) for a moment. But I think the big wins for me do come from
the other existing lib/ stuff, along with vtysh configuration.

> > So I'm asking if there's any additional thoughts to this? I'm
> > offering to write the netlink code properly.
> 
> Go ahead - znl_* should be OK :)
> 
> (Please try to make them as idiot-proof as possible, e.g. misuse of
> the API should produce compiler warnings where possible.  Printing
> netlink packets OTOH isn't absolutely neccessary since there's nlmon
> in kernel now.)

Ok. I'll take stab at it. I can first implement whatever I'm doing, and
then as a later commit convert other existing netlink stuff to use it
to make migration / review simpler.

/Timo





More information about the Quagga-dev mailing list