Laine Stump wrote:

On 12/23/2012 07:54 AM, Roman Bogorodskiy wrote:

...

Hi,

Few weeks ago when I have submitted my all-in-one huge FreeBSD, Eric made some comments on the networking part:

http://www.redhat.com/archives/libvir-list/2012-December/msg00432.html

Now when we're done with most of the other things, I'd like to discuss networking in more detail.

Basically, the main question that bothers me is what would be the best way to refactor network/bridge_driver.c?

Currently it contains a number of things:

- dnsmasq routines, static, but cross-platform, doesn't need any changes (at least major ones) - the same for radvd - minor firewalld bits, that are linux specifc - iptables routines. There are quite a lot of that code and it's also gets called from many places in the file

Note that there is already viriptables.c in src/util. Of course those are lower level primitives, but they were really created with the needs of bridge_driver.c in mind.

...

I'm thinking about an approach that Eric mentioned for virthread.c:

- keep dnsmasq and radvd in bridge_driver.c - move out all linux specific code to bridge_driver_linux.c and #include it in a way similar to virthread.c

I must have missed that message. I dislike #including .c files, if that's what you're talking about.

...

- in a similar way create bridge_driver_freebsd.c

Does it sound reasonable?

I don't mind the idea of bridge_driver_xxx.c and bridge_driver.c, but I think that the appropriate bridge_driver_xxx.c should be added to the Makefile rather than #included in bridge_driver.c.

I would do it by having a common .h file bridge_driver_platform.h which had a set of functions that must be defined for any supported platform, then rewrite bridge_driver.c so that everything that isn't platform-independent is turned into a call to a function or functions declared in bridge_driver_platform.h and defined in bridge_driver_(linux|freebsd).c

For example, the firewalld/dbus specific stuff can be considered as "an opaque chunk of Linux specific code that must be executed during networkStartup()". I think that can be taken care of by declaring a function networkStartupPlatform() in bridge_driver_platform.h, then defining it in both bridge_driver_linux.c and bridge_driver_freebsd.c (where it may simply return success.)

All of the seemingly iptables-specific functions bridge_driver.c should be renamed to use a more generic term in place of "iptables":

networkAddIpSpecificIptablesRules networkAddMasqueradingFilterRules networkAddRoutingFilterRules networkReloadFilterRules networkRemoveFilterRules networkRemoveIpSPecificFilterRules networkRemoveGeneralFilterRules [etc]

Almost all of those functions should be able to remain in bridge_driver.c; only the bottom-level functions that are directly calling iptables(Add|Remove)*() functions should need to be moved to bridge_driver_xxx.c (and re-written appropriately for FreeBSD). (Or maybe the concepts behind the functions declared in src/util/viriptables.h are generic enough that those functions should also be renamed, and a new src/util/virpf.c (or whatever filtering package is used on FreeeBSD these days - I haven't kept up with BSD for a long time) defined that implements these filter*() functions for that platform.

That sounds reasonable, I'll start moving this way. Thanks Roman Bogorodskiy