Re: [libvirt] [RFC] Refactoring bridge driver for portability

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.