Re: filters: strange protocol restarts

[Ondrej Zajicek]

On Mon, Sep 30, 2013 at 09:21:02PM +0300, Sergey Popovich wrote:
 I start new patch series based on this thread with changes to filtering
 code. I hope BIRD developers and community found useful these patches.

Thanks, i will comment each patch file independently. Your patches seems
to be somewhat malformed, probably reformatted by e-mail client. I prefer
receiving patches as attachments.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
To err is human -- to blame it on a computer is even more so.


signature.asc
Description: Digital signature

Re: filters: strange protocol restarts

[Sergey Popovich]

В письме от 3 октября 2013 12:57:45 Вы написали:
 On Mon, Sep 30, 2013 at 09:21:02PM +0300, Sergey Popovich wrote:
  I start new patch series based on this thread with changes to filtering
  code. I hope BIRD developers and community found useful these patches.
 
 Thanks, i will comment each patch file independently. Your patches seems
 to be somewhat malformed, probably reformatted by e-mail client. I prefer
 receiving patches as attachments.

Yes sorry for that, thats new e-mail agent. I will send next patches
as attachment.

Thanks for your interest to my patches. 

-- 
SP5474-RIPE
Sergey Popovich

Re: filters: strange protocol restarts

[Sergey Popovich]

В письме от 27 сентября 2013 17:07:55 пользователь Ondrej Zajicek написал:
 It is documented:
 
 Special operators include cf/tilde;/ for is element of a set
 operation - it can be used on element and set of elements of the same
 type (returning true if element is contained in the given set), ...
 or on clist and pair/quad set (returning true if there is an element of
 the clist that is also a member of the pair/quad set)
 

Yes, really. But is not clear to me how to use this to test for
community/ext community is non-empty.

 Well, that was my first idea w.r.t. the original problem with reconfigure,
 but having nonsensical '' offends my algebraic sense ;-) and there is no
 good reason to imlement some consistent arbitrary ordering on such objects.
 
 Good solution would be to have val_same(), which would work on all types
 (and would be used where equality is tested), while val_compare would only
 work on ordered objects.
 
 You are right that pm_path_compare() already returns some nonsense.
 

Thats my first idea. See [PATCH 03/12] in series.

 Well, these constants are not documented on purpose, as they are more
 like ad-hoc constants for testing purposes, not much needed when you
 have X.empty operator.

Sure, no problem with ad-hoc constants, let them be *internal*. I just fix
comparison/assignment with these constants in [PATCH 03/12].

 X.empty operator could be documented, i wanted to do it already,
 the only problem i have with it is its name. When i see 'X.empty',
 i would think that 'empty' mainly as adjective (predicate
 for emptiness-testing), not as verb (operator of emptying).
 
 Having some predicate that would return true/false whether the
 path/list/set is empty would be useful, but both cannot be named
 'empty'. Perhaps name the former 'reset'? Or keep current 'empty' and
 use some other name for the predicate? Any comments on this?

No problem with name 'reset', but I think we should leave 'empty' for
extended attributes for compatibility and document only 'reset'.
This was done in [PATCH 06/12]. To determine if clist or eclist
is empty, I suggest to use 'len' operator as this currently implemented
for bgppath type, which is done in [PATCH 05/12].

I start new patch series based on this thread with changes to filtering
code. I hope BIRD developers and community found useful these patches.

-- 
SP5474-RIPE
Sergey Popovich

Re: filters: strange protocol restarts

[Sergey Popovich]

В письме от 21 августа 2013 22:58:48 Вы написали:
 Среда, 21 августа 2013, 20:46 +02:00 от Ondrej Zajicek 
santi...@crfreenet.org:

  Can some one explain to me this behavior of BIRD? Does this mean
  that usage of constats with type prefix set is not recommended?
 
 Hello
 
 This is just a stupid mistake. Sets were not properly compared.
 Use attached patch.
 
 Huge thanks! Tested and found working!
 

There are at least one other caveat related to comparison, not covered by this 
patch:

How to determine if clist, eclist are empty? Statements like
  bgp_community_list = -empty- or
  bgp_ext_community_list = --empty--
gives filter runtime error. Same reason for comparing bgppath type
with +empty+ (i.e. bgp_path = +empty+), however for this we could
use something like bgp_path.len = 0)?
 
Interesting thing about comparison I found in filter/test.conf2:
  clist ~ [(*,*)]
but this is not documented.

So I consider to submit my attempt to address this caveat and add some other 
useful things related to comparison:

  * Move same_tree() for SET comparison and trie_same() for PREFIX_SET 
comparison into val_compare(). Now statements like
bgp_community_list = -empty- and bgp_ext_community = --empty-- works.
I think this could be done as there at least one function, that violates
return code of val_compare() (-1,0,1): pm_path_compare().

  * Intdodoce list_compare() to compare clist and eclist as arrays of u32, not 
much useful, but I think it does not broke something. This is done to 
extend val_compare() to handle all known filter types (probably).

  * Introduce path_compare() to compare bgppath type. Compares path length,
and element by element, considering AS_PATH_SET more longer. Reason
for adding same as with list_compare().

  * Add minor optimisations on speed when comparing with int_cmp(), 
uint_cmp(), u64_cmp(), by removing branching.

  * Split comparison to T_VOID in val_compare() in two parts, when types are 
equal (add case to switch) and when are not. Do similar in val_in_range().

  * Document -empty-, --empty-- and +empty+ special purpose constants.
Also document S.empty statement for use with dynamic attributes.

Another patch, just adds some other possibilities to check length of clist, 
eclist and ip address. Last could be used to determine AFI for which bird is 
build (see example function in documentation section on patch).

Patches tested, and found working, at least for me.

-- 
SP5474-RIPE
Sergey Popovichdiff -purN a/doc/bird.sgml b/doc/bird.sgml
--- a/doc/bird.sgml	2013-09-25 12:09:39.0 +0300
+++ b/doc/bird.sgml	2013-09-25 12:21:33.997680313 +0300
@@ -1038,6 +1038,11 @@ incompatible with each other (that is to
   cfm/P/.prepend(m/A/);/cf if m/P/ is appropriate route attribute
   (for example cf/bgp_path/).
 
+	  Additionaly operator cfm/P/.empty/cf empties list of autonomous systems if
+	  m/P/ is appropriate route attribute (for example bgp_path). This is equivalent to
+	  statement cfm/P/ = +empty+;/cf where cf/+empty+/ represents empty bgppath and
+	  could be used in conditional expressions to match empty list of autonomous systems.
+
 	tag/bgpmask/
 	  BGP masks are patterns used for BGP path matching
 	  (using cfpath tilde; [= 2 3 5 * =]/cf syntax). The masks
@@ -1082,13 +1087,22 @@ incompatible with each other (that is to
   attribute (for example cf/bgp_community/). Similarly for
   cf/delete/ and cf/filter/.
 
+	  Additionaly operator cfm/C/.empty/cf empties appropriate route
+	  attribute m/C/ (for example cf/bgp_community/), which is
+	  equivalent to statement cfm/C/ = -empty-;/cf where
+	  cf/-empty-/ represents empty clist and could be used in
+	  conditional expressions to determine if clist variable is empty,
+	  as cfdefined()/cf always succeed on clist even if variable is
+	  not defined.
+
 	tag/eclist/
 	  Eclist is a data type used for BGP extended community lists.
 	  Eclists are very similar to clists, but they are sets of ECs
 	  instead of pairs. The same operations (like cf/add/,
 	  cf/delete/, or cf/tilde;/ membership operator) can be
 	  used to modify or test eclists, with ECs instead of pairs as
-	  arguments.
+	  arguments. For extended communities cf/--empty--/ represents
+	  empty eclist and could be used similarly to cf/-empty-/ in clist.
 /descrip
 
 sectOperators
diff -purN a/filter/filter.c b/filter/filter.c
--- a/filter/filter.c	2013-09-25 12:10:45.0 +0300
+++ b/filter/filter.c	2013-09-25 12:12:34.215026255 +0300
@@ -112,25 +112,107 @@ pm_format(struct f_path_mask *p, byte *b
   *buf = 0;
 }
 
-static inline int int_cmp(int i1, int i2)
+static inline int
+int_cmp(int i1, int i2)
+{
+  return (i1  i2) - (i1  i2);
+}
+
+static inline int
+uint_cmp(unsigned int i1, unsigned int i2)
 {
-  if (i1 == i2) return 0;
-  if (i1  i2) return -1;
-  else return 1;
+  return (i1  i2) - (i1  i2);
 }
 
-static inline int 

Re: filters: strange protocol restarts

[Ondrej Zajicek]

On Fri, Sep 27, 2013 at 02:35:45PM +0300, Sergey Popovich wrote:
 There are at least one other caveat related to comparison, not covered by 
 this 
 patch:
 
 How to determine if clist, eclist are empty? Statements like
   bgp_community_list = -empty- or
   bgp_ext_community_list = --empty--
 gives filter runtime error. Same reason for comparing bgppath type
 with +empty+ (i.e. bgp_path = +empty+), however for this we could
 use something like bgp_path.len = 0)?
  
 Interesting thing about comparison I found in filter/test.conf2:
   clist ~ [(*,*)]
 but this is not documented.

It is documented:

Special operators include cf/tilde;/ for is element of a set
operation - it can be used on element and set of elements of the same
type (returning true if element is contained in the given set), ...
or on clist and pair/quad set (returning true if there is an element of
the clist that is also a member of the pair/quad set)

 So I consider to submit my attempt to address this caveat and add some other 
 useful things related to comparison:
 
   * Move same_tree() for SET comparison and trie_same() for PREFIX_SET 
 comparison into val_compare().

Well, that was my first idea w.r.t. the original problem with reconfigure, but
having nonsensical '' offends my algebraic sense ;-) and there is no good
reason to imlement some consistent arbitrary ordering on such objects.

Good solution would be to have val_same(), which would work on all types
(and would be used where equality is tested), while val_compare would only
work on ordered objects.

You are right that pm_path_compare() already returns some nonsense.

   * Split comparison to T_VOID in val_compare() in two parts, when types are 
 equal (add case to switch) and when are not. Do similar in val_in_range().
 
   * Document -empty-, --empty-- and +empty+ special purpose constants.
 Also document S.empty statement for use with dynamic attributes.

Well, these constants are not documented on purpose, as they are more
like ad-hoc constants for testing purposes, not much needed when you
have X.empty operator.

X.empty operator could be documented, i wanted to do it already,
the only problem i have with it is its name. When i see 'X.empty',
i would think that 'empty' mainly as adjective (predicate
for emptiness-testing), not as verb (operator of emptying).

Having some predicate that would return true/false whether the
path/list/set is empty would be useful, but both cannot be named
'empty'. Perhaps name the former 'reset'? Or keep current 'empty' and
use some other name for the predicate? Any comments on this?


-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
To err is human -- to blame it on a computer is even more so.


signature.asc
Description: Digital signature