Hi Ondrej,
Thanks for the feedback.
On 1/6/20 3:16 AM, Ondrej Zajicek wrote:
Thanks for the patchset, sorry for not giving feedback sooner, was on
vacation.
No problem, I was expecting that. I am also first responding now, since I
have been under the weather for the past few days.
If i understand it correctly, the main point of this patchset is to
introduce 'typed symbols', which have multiple values based of network
type of processed route.
Correct. The patches allows for explicitly typed constants, filters and
functions. If a filter or function uses a typed symbol, it will implicitly
be typed itself. Once the context is know that version of the filter will
be generated.
I must say that i do not like the approach taken in the patch, seems to
me that for intended purpose it is too intrusive change and the feature
is too idiosyncratic - i don't recall any language where a constant may
represent multiple unrelated values based on context.
I agree that the patch set is a little intrusive, but I decided that it
was good enough for a PoC and to get the discussion started, hence
labeling it as an RFC patch.
In bird 1 it was possible to have bird4.conf and bird6.conf share common
configuration files, but have 2 different files defining symbols.
In bird 2 this ends out with a lot of symbols ending in "4" or "6",
and quite a few case statements or to do address family specific processing.
The case statements could be hidden behind a case-function per data type.
function get_typed_pfx_set(prefix set v4; prefix set v6)
{
case net.type {
NET_IP4: return v4;
NET_IP6: return v6;
}
}
Even this leads to long lines like: get_typed_pfx_set(AS65000_prefixes4,
AS65000_prefixes6),
including the need to pass both prefix sets, through generic filtering
functions.
Overall leading to an idiosyncratic config.
I would hope that people would only use this feature to store related values,
however I have only enforced that they must have the same data type.
It is also over-specific (dispatch hardcoded for net_type, while there
are other, context-specific properties, like protocol type).
I made the syntax and symbol part very net_type specific, since I'm not aware
of other context parameters where it would make a lot of sense.
I specifically choose not allow a fallback value, ie. calling cf_error
if a symbol is not defined for a given net_type.
With protocol typing I see it as more useful for protocol specific
overrides, so maybe a fallback value should be allowed?
Does anybody else have any feedback on this?
One way to make it less intrusive would be to keep symbol table the same
(just symbol->value mapping) and do the change just in 'value' part.
I don't have a problem with converting it to a linked list of versions of the
symbol.
You have this example:
define default_route:ipv4 = 0.0.0.0/0;
define default_route:ipv6 = ::/0;
This could be done by using function instead of constant:
function default_route()
{
case net.type
{
NET_IP4: return 0.0.0.0/0;
NET_IP6: return ::/0;
}
}
>
I agree that it is more cumbersome than your version.
I used default route mostly as an easy to understand example, the big
benefit comes with prefix filtering hence the reference to bgpfilterguide.
Seems to me that your intended objective could be achieved, while keeping
it universal and reasonable, by several independent changes:
1) Allow 'lazy-evaluated constants', that could depend on context. These
are essentially zero-argument functions in disguise.
>
2) Allow something like if/case expressions (instead of just statements).
3) (optional) partially-evaluate/adapt filters based on context.
When we originally discussed this idea during RIPE78 in Reykjavik, we also
talked about that it should be possible to optimize it parse time.
Therefore I wanted to explore the feasibility of doing that, the shortcut
I took here, by only having a thin adaptation layer, is that I had to mark
it as never constant. That could be fixed by moving most of the linearization
work to the adaptation state, leaving only a loop check the current
linearization stage.
The benefit of doing the adaptation at parse time, means that non-existent
variants of a symbol can be revealed before a new config is loaded.
The redundant case statements during run-time is not an issue for us,
but I imagine it could be to some of the route server operators.
So one could wrote something like:
define default_route = case net.type {
NET_IP4: 0.0.0.0/0;
NET_IP6: ::/0;
}
This syntax could work, but should just result in the entries being put in
a linked-list under the symbol, so that it can be guaranteed that they all have
the same type.
I prefer to be able to define constants as several individual statements.
If constants can be defined for other parameters than net_type, should then
also be possible to do it multi-dimensional? Fx. for ipv4 in protocol x.
(The remaining wart is a current style of enum constants, like 'NET_IP4'.)
I'm aware that net.type uses the C constants, but I choose to use `net_type`,
since it is consistent with table and channel type definitions.
--
Best regards
Asbjørn Sloth Tønnesen
