Hi everyone, Comments inline.
On Mon, Jul 21, 2014 at 2:29 PM, Ondrej Zajicek <[email protected]> wrote: > On Sun, Jul 20, 2014 at 12:05:39PM +0200, Eloi Carbó Solé wrote: > > Hi everyone, > > > > I am Eloi Carbó, a student from the UPC in Catalonia, doing a Google > Summer > > of Code [0] with the QMP project [1] team, and the support of Guifi.net > [2] > > and BMX6 [3] developers and under the scope of Freifunk [4]. > > > > I would like to inform you that we are bringing UCI and LUCI > configuration > > support to Bird4 (now) and Bird6 (ASAP) putting our efforts on BGP > routing. > > Hello > > It would be nice to have UCI configuration in OpenWRT for BIRD. Although > I am not sure how filters could be usefully represented in UCI and these > are pretty important in most nontrivial BGP configs. > Filters (and functions) have already been discussed and I am going to implement them in files (ie: {path}/filters/Filter-Name) because, as you have already said, they have a complex structure that do not fit in UCI. These files will have the actual syntax of filters in Bird and will be included during the execution of init.d script. Filters and functions will be implemented soon. > > I would suggest to keep structure of UCI BIRD configs consistent with > native BIRD configs. Although BIRD sometimes uses multiple nesting and i > am not sure whether UCI can represent that. Also, please use same option > names (when possible) with underscore separator for space instead of > camelCase (e.g. scan_time instead of scanTime). That would be consistent > with both UCI style and BIRD style. > > I was following the same naming UCI scheme than in my previous project with UCI but I can surelly change this naming to a_b. I attach you in the final of the mail a piratepad with an example applying the changes you asked. > > Some comments to /etc/config/bird4: > > config bird 'bird' > option useUCIconfig '0' > #Caution! Enabling this option, Bird will translate this > #UCI file and use it instead of /etc/bird4.conf > option UCIconfigFile '/tmp/bird4.conf' > > Perhaps '/var/etc/bird4.conf' as default ? > > The configuration is created by init.d script and copied to /tmp which is in RAM following the OpenWRT working methodology. > > config global 'global' > option logFile '/tmp/bird.log' > > Default configuration should probably use syslog instead of logfile. > There is no syslog in OpenWRT and for that reason I am using log files in RAM (again, user can modify this directory). > option log 'all' > option debug 'all' > > 'debug all' is not a good default value. That would generate tons of log > messages. > > I will change it in the next commit using 'off' following the default value in the documentation. > option table 'arf' > > 'table' should not be a global option, more like one section for each table > (although there is just one option 'sorted' and tables currently do not > have block for options in BIRD config, but that will change in the future). > > This was a mistake. As you say, there can be 'N' table routes so, using the 'option table', only one extra table could be created. Find in my attach to piratepad the modifications done. > > config kernel kernel1 > option tablePriority '100' > > This is misnomer. It is not a table priority, just a kernel table ID. > > Yes, it was a mistake naming the option. Changed! > > config device device1 > option scanTime '10' > #list primaryIface 'eth0:192.168.1.1' > #list primary '192.168.0.0/16' > > You may ignore 'primary' device option. It is mostly deprecated and not > much useful. > > Changed. Then scan_time is the only one option available for this protocol? > > config bgp bgp1 > option table 'arf' > option import 'all' > option export 'all' > option as '65001' > option disabled '0' > > config neighbor > option type 'bgp' > option instance 'bgp1' > option addr '172.16.1.5' > option as '65530' > > In BIRD, we don't have separate neighbors as subsections of BGP. We have > one BGP instance per neighbor (also with separate protocol name). UCI > structure should probably reflect that too. If you want common options > for multiple BGP neighbors, you could use protocol templates for BGP: > > > > config bgp_template bgp_common > option table 'arf' > option import 'all' > option export 'all' > option locas_address '172.16.1.1' > option local_as '65001' > > > config bgp bgp1 > option template 'bgp_common' > option neighbor_address '172.16.1.5' > option neighbor_as '65530' > > I was going to implement templates later but your idea makes sense and I will adopt it instead of the actual one. The reason of having neighbors as sections was to treat them apart, as they may be used for bgp or ospf. > > config route > option instance 'static1' > option type 'general' > option addr '192.168.9.0/24' > list via '10.99.105.159' > > Perhaps not use 'list' but 'option' for 'via' in this case. > This was a mistake. Changed! > Type should be 'router' which should be default type. > > The problem with 'route's is that there are 5 different types so I had to name somehow them. If there is a specific naming for them besides the 'general->router' but this naming will be hidden to the user, as it is only a internal init.d configuration value. Users will see this Piratepad <http://piratepad.net/Rfmb8nXiRp>. option 'addr' should be named 'prefix' instead. > > Changed! > > config route > option instance 'static1' > option type 'special' > option addr '192.168.2.0/24' > option attribute 'unreachable' > > It is unnecessary to use separate 'attribute' option. You could use > 'unreachable', 'blackhole' and 'prohibit' as route types. > > The type option is used by the init.d configuration to differentiate with the 5 different routes: 'general->router' which have only a prefix and ip. 'multipath' with a list of 'via's, some with weight. 'special' with an attribute (as in documentation: blackhole|unreachable|prohibit). Following your idea, I would have 4 + 3 new types. 'iface' with a prefix and interface. 'recursive' only with an IP. > config route > option instance 'static1' > option type 'multipath' > option addr '192.168.30.0/24' > list via '172.16.1.5' > list via '172.16.1.6' > list viaWeight '172.16.1.2:70' > > Weight attribute is probably not important enough to implement it with > such ugly syntax. > > I understand correctly that you do not like the naming or maybe that is an unused option that could be left for the moment? > > For /etc/init.d/bird4, please use 'case' instead of 'if/elif' sequences. > > I was following some OpenWRT packages using UCI as examples and they used if/elseif. I can change it to 'case' for sure. > > -- > Elen sila lumenn' omentielvo > > Ondrej 'Santiago' Zajicek (email: [email protected]) > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) > "To err is human -- to blame it on a computer is even more so." > To sum up, many thanks for your feedback, it is really appreciated. As I told, here it is a Piratepad <http://piratepad.net/xOaveQLlyh> with an example of a proposed configuration. Feel free to make changes or comments in the document and, if you find it correct, I will start to fix the init.d script to fit in it. Best regards, -- Eloi Carbó
