constant for empty prefix set
Hi, I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ]; It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
Hi, Let me ping this. I would like to receive a feedback on this (and some other my recent) messages. On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
Hi, Let me remind about this once again. On Tue, Jan 11, 2022 at 1:10 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Let me ping this. I would like to receive a feedback on this (and some other my recent) messages.
On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
On Thu, Feb 10, 2022 at 02:09:59PM +0100, Alexander Zubkov wrote:
Hi,
Let me remind about this once again.
Hi Sorry for ignoring your previous mail, i needed to focus on finishing things for 2.0.9, now i can continue with your patches. 1) UDP logging - I have no principal issue with this, will review and merge. 2) Keeping proto state during reconfiguration - I am ok with 'configure keep' patch, will preview and merge. I do not want 'keep state' per-protocol option. Perhaps also some 'configure xyz' for regular configure and some global option how 'configure' without arguments should behave for people who would like 'configure keep' to be the default behavior. 3) prepend_times() - I am a bit ambivalent about that. I see usefulness of that. I do not like the name. It can be implemented as optional argument to prepend(), but not sure if that is better. Will think about that. Also, considering user mistakes like [*], we perhaps should limit multiplier by say 16. [*] https://bgpmon.net/long-as-paths-causing-commotion/ 4) Empty prefix set - Too hacky and inconsistent syntax. Note that there are also non-prefix sets. We already have ugly hacks for empty community lists, we want to avoid yet another way to define empty structures. Unfortunately there is no easy way to define literals that could be used for multiple data types. We already have A.B.C.D, that could be both IP address and router ID, and it is super annoying in the code. We would like to have some more generic solution, we will think about that. 5) Reduce/reduce fix - Will look at this.
On Tue, Jan 11, 2022 at 1:10 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Let me ping this. I would like to receive a feedback on this (and some other my recent) messages.
On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
-- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@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."
On Mon, Feb 14, 2022 at 9:47 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Feb 10, 2022 at 02:09:59PM +0100, Alexander Zubkov wrote:
Hi,
Let me remind about this once again.
Hi
Sorry for ignoring your previous mail, i needed to focus on finishing things for 2.0.9, now i can continue with your patches.
Thank you, looking forward to it!
1) UDP logging - I have no principal issue with this, will review and merge.
2) Keeping proto state during reconfiguration - I am ok with 'configure keep' patch, will preview and merge. I do not want 'keep state' per-protocol option. Perhaps also some 'configure xyz' for regular configure and some global option how 'configure' without arguments should behave for people who would like 'configure keep' to be the default behavior.
3) prepend_times() - I am a bit ambivalent about that. I see usefulness of that. I do not like the name. It can be implemented as optional argument to prepend(), but not sure if that is better. Will think about that. Also, considering user mistakes like [*], we perhaps should limit multiplier by say 16.
I do not insist on the name of course, or implementation. :)
[*] https://bgpmon.net/long-as-paths-causing-commotion/
4) Empty prefix set - Too hacky and inconsistent syntax. Note that there are also non-prefix sets. We already have ugly hacks for empty community lists, we want to avoid yet another way to define empty structures.
Yes, I know about other sets, that is why I choose how to name this set. But probably not completely the same: /empty/ vs [ /empty/ ] I did not know there was an intention to get rid of those hacky names, but on the other hand they need to be defined somehow and just using "[]" does not work because it is ambiguous for the parser. I do not insist on the name here, of course, just on the idea itself. Possibility to define an empty prefix set is indeed useful. If I match network against some named set, than I have no variants now how to define such set to be empty (or I do not know about such variant). If such set is defined separately, for example dynamically, sometimes I want it to be empty, so nothing would match it.
Unfortunately there is no easy way to define literals that could be used for multiple data types. We already have A.B.C.D, that could be both IP address and router ID, and it is super annoying in the code. We would like to have some more generic solution, we will think about that.
Yes, I understand. Generic solution of course makes things clearer. But in this case even if we have some means to specify that ip-like literal is actually of type ip or quad, anyway it would be not so good to force everybody to specify types for all ip-like literals in their configs. As for generic way to define empty sets/list, I can imagine now these variants for consideration: function-like: emtpy_list(<type>), empty_set(<type>) - example: empty_set(ip), empty_list(lc), ... empty(<type>) - example: empty(ip set), empty(lclist) type-conversion-like: (<type>) [] - example: (ip set) [], (lclist) [] OO-like: type.<type>.empty - type.ip_set.empty, type.lclist.empty
5) Reduce/reduce fix - Will look at this.
On Tue, Jan 11, 2022 at 1:10 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Let me ping this. I would like to receive a feedback on this (and some other my recent) messages.
On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions. It could be a separate type (for example T_EMPTY_SET) or a set (T_SET) with an empty tree. I can try to prepare patches for that if you want. On Mon, Feb 14, 2022 at 11:46 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Feb 14, 2022 at 9:47 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Feb 10, 2022 at 02:09:59PM +0100, Alexander Zubkov wrote:
Hi,
Let me remind about this once again.
Hi
Sorry for ignoring your previous mail, i needed to focus on finishing things for 2.0.9, now i can continue with your patches.
Thank you, looking forward to it!
1) UDP logging - I have no principal issue with this, will review and merge.
2) Keeping proto state during reconfiguration - I am ok with 'configure keep' patch, will preview and merge. I do not want 'keep state' per-protocol option. Perhaps also some 'configure xyz' for regular configure and some global option how 'configure' without arguments should behave for people who would like 'configure keep' to be the default behavior.
3) prepend_times() - I am a bit ambivalent about that. I see usefulness of that. I do not like the name. It can be implemented as optional argument to prepend(), but not sure if that is better. Will think about that. Also, considering user mistakes like [*], we perhaps should limit multiplier by say 16.
I do not insist on the name of course, or implementation. :)
[*] https://bgpmon.net/long-as-paths-causing-commotion/
4) Empty prefix set - Too hacky and inconsistent syntax. Note that there are also non-prefix sets. We already have ugly hacks for empty community lists, we want to avoid yet another way to define empty structures.
Yes, I know about other sets, that is why I choose how to name this set. But probably not completely the same: /empty/ vs [ /empty/ ] I did not know there was an intention to get rid of those hacky names, but on the other hand they need to be defined somehow and just using "[]" does not work because it is ambiguous for the parser. I do not insist on the name here, of course, just on the idea itself. Possibility to define an empty prefix set is indeed useful. If I match network against some named set, than I have no variants now how to define such set to be empty (or I do not know about such variant). If such set is defined separately, for example dynamically, sometimes I want it to be empty, so nothing would match it.
Unfortunately there is no easy way to define literals that could be used for multiple data types. We already have A.B.C.D, that could be both IP address and router ID, and it is super annoying in the code. We would like to have some more generic solution, we will think about that.
Yes, I understand. Generic solution of course makes things clearer. But in this case even if we have some means to specify that ip-like literal is actually of type ip or quad, anyway it would be not so good to force everybody to specify types for all ip-like literals in their configs.
As for generic way to define empty sets/list, I can imagine now these variants for consideration:
function-like: emtpy_list(<type>), empty_set(<type>) - example: empty_set(ip), empty_list(lc), ... empty(<type>) - example: empty(ip set), empty(lclist)
type-conversion-like: (<type>) [] - example: (ip set) [], (lclist) []
OO-like: type.<type>.empty - type.ip_set.empty, type.lclist.empty
5) Reduce/reduce fix - Will look at this.
On Tue, Jan 11, 2022 at 1:10 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Let me ping this. I would like to receive a feedback on this (and some other my recent) messages.
On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
Made some example changes to allow the use of an empty set, which works with all types and prefixes too. Its value has type T_SET with NULL pointer to the tree. Looks goot at the first sight. The only issue I know about now is with function as_path_filter(...) in nest/a-path.c. It has both pointer to struct f_tree and integer number (to compare ASes with) in its arguments. And it uses pointer if it is not null and the number otherwise. So it cannot distinguish if it was provided with an empty set, and uses the number in such cases. It will still match 0 in the path, when it should match nothing. So further intervention to this function is required. I have added two commented tests into test.conf which are passing now, but should not: #bt_assert(delete(prepend(prepend(+empty+, 0), 1), []) = prepend(+empty+, 1)); #bt_assert(filter(prepend(prepend(+empty+, 0), 1), []) = prepend(+empty+, 0)); On Tue, Feb 15, 2022 at 12:09 PM Alexander Zubkov <green@qrator.net> wrote:
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions. It could be a separate type (for example T_EMPTY_SET) or a set (T_SET) with an empty tree. I can try to prepare patches for that if you want.
On Mon, Feb 14, 2022 at 11:46 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Feb 14, 2022 at 9:47 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Feb 10, 2022 at 02:09:59PM +0100, Alexander Zubkov wrote:
Hi,
Let me remind about this once again.
Hi
Sorry for ignoring your previous mail, i needed to focus on finishing things for 2.0.9, now i can continue with your patches.
Thank you, looking forward to it!
1) UDP logging - I have no principal issue with this, will review and merge.
2) Keeping proto state during reconfiguration - I am ok with 'configure keep' patch, will preview and merge. I do not want 'keep state' per-protocol option. Perhaps also some 'configure xyz' for regular configure and some global option how 'configure' without arguments should behave for people who would like 'configure keep' to be the default behavior.
3) prepend_times() - I am a bit ambivalent about that. I see usefulness of that. I do not like the name. It can be implemented as optional argument to prepend(), but not sure if that is better. Will think about that. Also, considering user mistakes like [*], we perhaps should limit multiplier by say 16.
I do not insist on the name of course, or implementation. :)
[*] https://bgpmon.net/long-as-paths-causing-commotion/
4) Empty prefix set - Too hacky and inconsistent syntax. Note that there are also non-prefix sets. We already have ugly hacks for empty community lists, we want to avoid yet another way to define empty structures.
Yes, I know about other sets, that is why I choose how to name this set. But probably not completely the same: /empty/ vs [ /empty/ ] I did not know there was an intention to get rid of those hacky names, but on the other hand they need to be defined somehow and just using "[]" does not work because it is ambiguous for the parser. I do not insist on the name here, of course, just on the idea itself. Possibility to define an empty prefix set is indeed useful. If I match network against some named set, than I have no variants now how to define such set to be empty (or I do not know about such variant). If such set is defined separately, for example dynamically, sometimes I want it to be empty, so nothing would match it.
Unfortunately there is no easy way to define literals that could be used for multiple data types. We already have A.B.C.D, that could be both IP address and router ID, and it is super annoying in the code. We would like to have some more generic solution, we will think about that.
Yes, I understand. Generic solution of course makes things clearer. But in this case even if we have some means to specify that ip-like literal is actually of type ip or quad, anyway it would be not so good to force everybody to specify types for all ip-like literals in their configs.
As for generic way to define empty sets/list, I can imagine now these variants for consideration:
function-like: emtpy_list(<type>), empty_set(<type>) - example: empty_set(ip), empty_list(lc), ... empty(<type>) - example: empty(ip set), empty(lclist)
type-conversion-like: (<type>) [] - example: (ip set) [], (lclist) []
OO-like: type.<type>.empty - type.ip_set.empty, type.lclist.empty
5) Reduce/reduce fix - Will look at this.
On Tue, Jan 11, 2022 at 1:10 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Let me ping this. I would like to receive a feedback on this (and some other my recent) messages.
On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
On Mon, Feb 21, 2022 at 11:01:25PM +0100, Alexander Zubkov wrote:
Made some example changes to allow the use of an empty set, which works with all types and prefixes too. Its value has type T_SET with NULL pointer to the tree. Looks goot at the first sight.
Hi Is assigning [] to variable of 'prefix set' type or function argument working properly? Note that global constants do not have type declaration, but variables and function arguments have. Also to your previous question:
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions.
It breaks static type controls or at least make them more complicated. Note that our types for sets are already pretty broken - we have just T_SET and T_PREFIX_SET in implementation, although different types of sets are used in filter language (e.g. 'int set' in variable definition) and matching types are tested just in runtime (based on set->from.type). But let's assume we would fix this, we would have types T_INT_SET, I_IP_SET, ..., T_PREFIX_SET for each set type. Then defining untyped empty set means it would be a new type (T_EMPTY_SET) different from others, it would require some exceptions in type checks (e.g. FI_VAR_SET just checks that type of value is the same as type of variable) and many expressions that accepts or returns value of just one type now would accepts or returns values of two different types. We would have to extend type checking system for this or implement some concept of subtyping or type coercion. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@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."
On Tue, Feb 22, 2022, 01:21 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Feb 21, 2022 at 11:01:25PM +0100, Alexander Zubkov wrote:
Made some example changes to allow the use of an empty set, which works with all types and prefixes too. Its value has type T_SET with NULL pointer to the tree. Looks goot at the first sight.
Hi
Is assigning [] to variable of 'prefix set' type or function argument working properly?
Note that global constants do not have type declaration, but variables and function arguments have.
Good points. Will check that. Thanks.
Also to your previous question:
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions.
It breaks static type controls or at least make them more complicated. Note that our types for sets are already pretty broken - we have just T_SET and T_PREFIX_SET in implementation, although different types of sets are used in filter language (e.g. 'int set' in variable definition) and matching types are tested just in runtime (based on set->from.type). But let's assume we would fix this, we would have types T_INT_SET, I_IP_SET, ..., T_PREFIX_SET for each set type.
Then defining untyped empty set means it would be a new type (T_EMPTY_SET) different from others, it would require some exceptions in type checks (e.g. FI_VAR_SET just checks that type of value is the same as type of variable) and many expressions that accepts or returns value of just one type now would accepts or returns values of two different types. We would have to extend type checking system for this or implement some concept of subtyping or type coercion.
Sure, I had the same concerns about this idea with T_EMPTY_SET. But on the other hand, with an empty T_SET we still need to make an exception at least for prefixes. And if, as you said, there are more types of sets in the future, than there will be still more exceptions. So in some circumstances, I think, both ways will have around the same number of exceptions. But currently, the empty T_SET looks easier of course. So, are you against an untyped empty set constant given the mentioned problems? Or you are unhappy about the complexity of implementing and maintaining that idea, but anyway consider it acceptable?
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
On Tue, Feb 22, 2022 at 03:02:55AM +0100, Alexander Zubkov wrote:
Also to your previous question:
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions.
It breaks static type controls or at least make them more complicated. Note that our types for sets are already pretty broken - we have just T_SET and T_PREFIX_SET in implementation, although different types of sets are used in filter language (e.g. 'int set' in variable definition) and matching types are tested just in runtime (based on set->from.type). But let's assume we would fix this, we would have types T_INT_SET, I_IP_SET, ..., T_PREFIX_SET for each set type.
Then defining untyped empty set means it would be a new type (T_EMPTY_SET) different from others, it would require some exceptions in type checks (e.g. FI_VAR_SET just checks that type of value is the same as type of variable) and many expressions that accepts or returns value of just one type now would accepts or returns values of two different types. We would have to extend type checking system for this or implement some concept of subtyping or type coercion.
Sure, I had the same concerns about this idea with T_EMPTY_SET. But on the other hand, with an empty T_SET we still need to make an exception at least for prefixes. And if, as you said, there are more types of sets in the future, than there will be still more exceptions. So in some circumstances, I think, both ways will have around the same number of exceptions. But currently, the empty T_SET looks easier of course.
So, are you against an untyped empty set constant given the mentioned problems? Or you are unhappy about the complexity of implementing and maintaining that idea, but anyway consider it acceptable?
I checked how IP/QUAD situation is solved and it is cleaner than i thought. There is already constant promotion function f_const_promotion() that can be extended to handle empty sets. So seems to me that with the current state of BIRD code this is simplest approach: There should be two empty sets values, one for T_SET (which behaves like in your patch) and one for T_PREFIX_SET (which would be just empty trie). Constant [] would generate the T_SET empty constant, but f_const_promotion() would handle promotion of T_SET empty constant to T_PREFIX_SET empty constant when needed. That should fix issues with variables/arguments. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@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."
Adding new pathches (v2 - full patch, v1-v2 - incremental from previous): 1) Modified as_path_filter function - added additional parameter to choose between using set or key parameter. Now empty set does not work like a zero integer constant. Also thought about passing one struct f_val parameter instead of those 3 parameters, but that requires adding include of filter/data.h to nest/attrs.h, and that looks like causing a chicken and an egg problem. 2) Added the case of empty T_SET to be promoted to T_PREFIX_SET in f_const_promotion function. There is need to create an empty trie, I used "f_new_trie(cfg_mem, 0)", but not sure cfg_mem is good to use here. It works with simple tests. By the way, not completely sure that during promotion, the value that is overwrited, is a copy of the constant value (because it can be a global named constant) and not the original value itself. I made some tests and looks like that after assignment, the global constan value remains to be a T_SET, I hope this is not because of some optimization. 3) Made filter/delete to act the same on T_PATH. For some reason delete(path, integer) was allowed, but filter(path, integer) wasn't. There are actually still problems with types: 4) I found out that passing arguments to the function does not check actual types during runtime. So we can actually pass any type to a function and it will even work, given that operators applied to the variables are supported. 5) When prefix set variable is assigned an empty set, it is promoted to the T_PREFIX_SET, of course, but after that it becomes not equal to "[]", because the type is different. But that can be considered a feature, not a bug. 6) Because of (4), as function arguments are not checked and are not promoted, if we have an argument of type prefix set, and pass [] to that function, it will remain of type T_SET. And if we try to assign it to another prefix set variable, there will be a runtime error like this: bird: filters, line 12: Argument 1 of VAR_SET must be of type set, got type set This "feature" affects quads also. On Tue, Feb 22, 2022 at 5:18 AM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Feb 22, 2022 at 03:02:55AM +0100, Alexander Zubkov wrote:
Also to your previous question:
I also think now - why at all do we need a typed empty set? I think it is possible to add an untyped empty set, that will act as a "joker" with any types. It should fit better to the current syntax now without introducing a lot of new constructions.
It breaks static type controls or at least make them more complicated. Note that our types for sets are already pretty broken - we have just T_SET and T_PREFIX_SET in implementation, although different types of sets are used in filter language (e.g. 'int set' in variable definition) and matching types are tested just in runtime (based on set->from.type). But let's assume we would fix this, we would have types T_INT_SET, I_IP_SET, ..., T_PREFIX_SET for each set type.
Then defining untyped empty set means it would be a new type (T_EMPTY_SET) different from others, it would require some exceptions in type checks (e.g. FI_VAR_SET just checks that type of value is the same as type of variable) and many expressions that accepts or returns value of just one type now would accepts or returns values of two different types. We would have to extend type checking system for this or implement some concept of subtyping or type coercion.
Sure, I had the same concerns about this idea with T_EMPTY_SET. But on the other hand, with an empty T_SET we still need to make an exception at least for prefixes. And if, as you said, there are more types of sets in the future, than there will be still more exceptions. So in some circumstances, I think, both ways will have around the same number of exceptions. But currently, the empty T_SET looks easier of course.
So, are you against an untyped empty set constant given the mentioned problems? Or you are unhappy about the complexity of implementing and maintaining that idea, but anyway consider it acceptable?
I checked how IP/QUAD situation is solved and it is cleaner than i thought. There is already constant promotion function f_const_promotion() that can be extended to handle empty sets. So seems to me that with the current state of BIRD code this is simplest approach:
There should be two empty sets values, one for T_SET (which behaves like in your patch) and one for T_PREFIX_SET (which would be just empty trie). Constant [] would generate the T_SET empty constant, but f_const_promotion() would handle promotion of T_SET empty constant to T_PREFIX_SET empty constant when needed. That should fix issues with variables/arguments.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
On Tue, Feb 22, 2022 at 08:18:35PM +0100, Alexander Zubkov wrote:
Adding new pathches (v2 - full patch, v1-v2 - incremental from previous):
1) Modified as_path_filter function - added additional parameter to choose between using set or key parameter. Now empty set does not work like a zero integer constant. Also thought about passing one struct f_val parameter instead of those 3 parameters, but that requires adding include of filter/data.h to nest/attrs.h, and that looks like causing a chicken and an egg problem.
OK. Also you could pass f_val *, so you would avoid the include. Although 3 parameters are also fine.
2) Added the case of empty T_SET to be promoted to T_PREFIX_SET in f_const_promotion function. There is need to create an empty trie, I used "f_new_trie(cfg_mem, 0)", but not sure cfg_mem is good to use here. It works with simple tests.
One solution would be a global constant empty trie, so you can just set pointer to that. Alternative would be just have NULL like in T_SET, but that would mean to add several special cases to trie functions, so that is probably worse approach. The difference is that for that trees NULLs are naturally valid values, while tries have header.
By the way, not completely sure that during promotion, the value that is overwrited, is a copy of the constant value (because it can be a global named constant) and not the original value itself. I made some tests and looks like that after assignment, the global constan value remains to be a T_SET, I hope this is not because of some optimization.
Yes it is a copy stored in FI_CONSTANT instruction: decl.m4:488 struct f_val *c = &arg->i_FI_CONSTANT.val It is copied during parsing from the global symbol: config.Y:731 $$ = f_new_inst(FI_CONSTANT, *($1->val))
3) Made filter/delete to act the same on T_PATH. For some reason delete(path, integer) was allowed, but filter(path, integer) wasn't.
Nak. That is intentional and documented. And the same behavior is for T_*LIST. Both filter(path, integer) and filter(clist, pair) is pretty useless and would require extending existing *list-handling code just to do that.
There are actually still problems with types:
4) I found out that passing arguments to the function does not check actual types during runtime. So we can actually pass any type to a function and it will even work, given that operators applied to the variables are supported.
That is oversight on my side, there should be static typechecks that also do promotions. Will look at that.
5) When prefix set variable is assigned an empty set, it is promoted to the T_PREFIX_SET, of course, but after that it becomes not equal to "[]", because the type is different. But that can be considered a feature, not a bug.
Yes, if FI_EQ used ARG_SAME_TYPE(), the typecheck would force promotion, but our equality check accepts any pair of objects (and returns false for object of different types instead of failing). I will look into this by implementing some restricted variant of ARG_SAME_TYPE() that forces promotion but otherwise ignores mismatched types. (although one could argue that if we already know in parse time that comparison would fail due to different types, then something is wrong.)
6) Because of (4), as function arguments are not checked and are not promoted, if we have an argument of type prefix set, and pass [] to that function, it will remain of type T_SET. And if we try to assign it to another prefix set variable, there will be a runtime error like this:
bird: filters, line 12: Argument 1 of VAR_SET must be of type set, got type set
This "feature" affects quads also.
-- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@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."
Updated patch. Again full and incremental. On Wed, Feb 23, 2022 at 4:48 AM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Feb 22, 2022 at 08:18:35PM +0100, Alexander Zubkov wrote:
Adding new pathches (v2 - full patch, v1-v2 - incremental from previous):
1) Modified as_path_filter function - added additional parameter to choose between using set or key parameter. Now empty set does not work like a zero integer constant. Also thought about passing one struct f_val parameter instead of those 3 parameters, but that requires adding include of filter/data.h to nest/attrs.h, and that looks like causing a chicken and an egg problem.
OK. Also you could pass f_val *, so you would avoid the include. Although 3 parameters are also fine.
Yes, I chose an additional parameter because with a pointer it will be impossible to call the function with this parameter "inline".
2) Added the case of empty T_SET to be promoted to T_PREFIX_SET in f_const_promotion function. There is need to create an empty trie, I used "f_new_trie(cfg_mem, 0)", but not sure cfg_mem is good to use here. It works with simple tests.
One solution would be a global constant empty trie, so you can just set pointer to that. Alternative would be just have NULL like in T_SET, but that would mean to add several special cases to trie functions, so that is probably worse approach. The difference is that for that trees NULLs are naturally valid values, while tries have header.
OK, created a global constant. There is a pointer to the memory pool in the f_trie structure, but I believe it is not used in this case. It may be even good to have it set to NULL for this constant in case somebody tries to add elements to that trie.
By the way, not completely sure that during promotion, the value that is overwrited, is a copy of the constant value (because it can be a global named constant) and not the original value itself. I made some tests and looks like that after assignment, the global constan value remains to be a T_SET, I hope this is not because of some optimization.
Yes it is a copy stored in FI_CONSTANT instruction:
decl.m4:488 struct f_val *c = &arg->i_FI_CONSTANT.val
It is copied during parsing from the global symbol:
config.Y:731 $$ = f_new_inst(FI_CONSTANT, *($1->val))
Thanks, I looked at these too, but still had some doubts. :)
3) Made filter/delete to act the same on T_PATH. For some reason delete(path, integer) was allowed, but filter(path, integer) wasn't.
Nak. That is intentional and documented. And the same behavior is for T_*LIST. Both filter(path, integer) and filter(clist, pair) is pretty useless and would require extending existing *list-handling code just to do that.
Heh, did not check that in the documentation. :) OK, returned the original behaviour. But on the other hand, although it is useless, I see no problems with allowing it just for the symmetry.
There are actually still problems with types:
4) I found out that passing arguments to the function does not check actual types during runtime. So we can actually pass any type to a function and it will even work, given that operators applied to the variables are supported.
That is oversight on my side, there should be static typechecks that also do promotions. Will look at that.
5) When prefix set variable is assigned an empty set, it is promoted to the T_PREFIX_SET, of course, but after that it becomes not equal to "[]", because the type is different. But that can be considered a feature, not a bug.
Yes, if FI_EQ used ARG_SAME_TYPE(), the typecheck would force promotion, but our equality check accepts any pair of objects (and returns false for object of different types instead of failing).
I will look into this by implementing some restricted variant of ARG_SAME_TYPE() that forces promotion but otherwise ignores mismatched types.
The other possibility is to alter val_same()/val_compare() functions to handle that situation like for quads. It just the matter of what behaviour is more preferrable. Current patch has this variant implemented taking into account that all empty prefix sets use the same emtpy trie constant. But altering val_compare() will make the comaprison of typed variables with empty sets to be true, for example prefix set with int set. On the other hand, currently it does the same with different sets represented as T_SET. And as I understand, it is also possible to compare an ip set with a quad set.
(although one could argue that if we already know in parse time that comparison would fail due to different types, then something is wrong.)
6) Because of (4), as function arguments are not checked and are not promoted, if we have an argument of type prefix set, and pass [] to that function, it will remain of type T_SET. And if we try to assign it to another prefix set variable, there will be a runtime error like this:
bird: filters, line 12: Argument 1 of VAR_SET must be of type set, got type set
This "feature" affects quads also.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
On Wed, Feb 23, 2022 at 03:46:53PM +0100, Alexander Zubkov wrote:
Updated patch. Again full and incremental.
Thanks, will look at it.
One solution would be a global constant empty trie, so you can just set pointer to that. Alternative would be just have NULL like in T_SET, but that would mean to add several special cases to trie functions, so that is probably worse approach. The difference is that for that trees NULLs are naturally valid values, while tries have header.
OK, created a global constant. There is a pointer to the memory pool in the f_trie structure, but I believe it is not used in this case. It may be even good to have it set to NULL for this constant in case somebody tries to add elements to that trie.
Agreed.
5) When prefix set variable is assigned an empty set, it is promoted to the T_PREFIX_SET, of course, but after that it becomes not equal to "[]", because the type is different. But that can be considered a feature, not a bug.
Yes, if FI_EQ used ARG_SAME_TYPE(), the typecheck would force promotion, but our equality check accepts any pair of objects (and returns false for object of different types instead of failing).
I will look into this by implementing some restricted variant of ARG_SAME_TYPE() that forces promotion but otherwise ignores mismatched types.
The other possibility is to alter val_same()/val_compare() functions to handle that situation like for quads. It just the matter of what behaviour is more preferrable. Current patch has this variant implemented taking into account that all empty prefix sets use the same emtpy trie constant.
Well, our plan is to remove all these explicit IP<->quad conversions from the code and keep just constant promotions, which are handled uniformly by the static type mechanism. They are still here just because it is minor backward incompatible change (as constant promotion just promote constants, while these coversions convert values). So we do not want to add more such exceptions.
But altering val_compare() will make the comaprison of typed variables with empty sets to be true, for example prefix set with int set. On the other hand, currently it does the same with different sets represented as T_SET. And as I understand, it is also possible to compare an ip set with a quad set.
The fact that non-prefix sets all use T_SET and are not properly differentiated by different T_*_SET value is unfortunate limitation, but probably not worth fixing/handling now. If we turn away from sets to say *lists, we already have different values for empty clist / eclist / lclist (i.e. returning false for val_same()). If we ever implement proper literal constants for *lists, then these empty values would be represented by the same empty literal (instead of the current adhoc syntax). -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@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."
On Wed, Feb 23, 2022, 17:43 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Wed, Feb 23, 2022 at 03:46:53PM +0100, Alexander Zubkov wrote:
Updated patch. Again full and incremental.
Thanks, will look at it.
Good. Give me know if some additional work on the patch is needed.
One solution would be a global constant empty trie, so you can just set pointer to that. Alternative would be just have NULL like in T_SET, but that would mean to add several special cases to trie functions, so that is probably worse approach. The difference is that for that trees NULLs are naturally valid values, while tries have header.
OK, created a global constant. There is a pointer to the memory pool in the f_trie structure, but I believe it is not used in this case. It may be even good to have it set to NULL for this constant in case somebody tries to add elements to that trie.
Agreed.
5) When prefix set variable is assigned an empty set, it is promoted to the T_PREFIX_SET, of course, but after that it becomes not equal to "[]", because the type is different. But that can be considered a feature, not a bug.
Yes, if FI_EQ used ARG_SAME_TYPE(), the typecheck would force promotion, but our equality check accepts any pair of objects (and returns false for object of different types instead of failing).
I will look into this by implementing some restricted variant of ARG_SAME_TYPE() that forces promotion but otherwise ignores mismatched types.
The other possibility is to alter val_same()/val_compare() functions to handle that situation like for quads. It just the matter of what behaviour is more preferrable. Current patch has this variant implemented taking into account that all empty prefix sets use the same emtpy trie constant.
Well, our plan is to remove all these explicit IP<->quad conversions from the code and keep just constant promotions, which are handled uniformly by the static type mechanism. They are still here just because it is minor backward incompatible change (as constant promotion just promote constants, while these coversions convert values). So we do not want to add more such exceptions.
Ok, I get it. Yes, than your variant with promotion is more preferable. Fortunately this exception can be easily dropped from the patch or when promotion is implemented.
But altering val_compare() will make the comaprison of typed variables with empty sets to be true, for example prefix set with int set. On the other hand, currently it does the same with different sets represented as T_SET. And as I understand, it is also possible to compare an ip set with a quad set.
The fact that non-prefix sets all use T_SET and are not properly differentiated by different T_*_SET value is unfortunate limitation, but probably not worth fixing/handling now.
Yes, I do not see a big problem here either. As you said, tries to compare types, which are different syntactically in the first place, looks strange by itself. And can be prohibited at parse time if needed.
If we turn away from sets to say *lists, we already have different values for empty clist / eclist / lclist (i.e. returning false for val_same()). If we ever implement proper literal constants for *lists, then these empty values would be represented by the same empty literal (instead of the current adhoc syntax).
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
On Wed, Feb 23, 2022 at 07:09:33PM +0100, Alexander Zubkov wrote:
On Wed, Feb 23, 2022, 17:43 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Wed, Feb 23, 2022 at 03:46:53PM +0100, Alexander Zubkov wrote:
Updated patch. Again full and incremental.
Thanks, will look at it.
Good. Give me know if some additional work on the patch is needed.
Thanks, merged: https://gitlab.nic.cz/labs/bird/-/commit/931c6ff51a312f005468f195cc2d27061d3... I removed the explicit conversion in val_compare(), moved as_path_filter() changes to a separate patch and changed it to use struct f_val (similar functions for *lists also use that). Already added typechecks for function call, which fixes issue when '[]' is passed as prefix set arg: https://gitlab.nic.cz/labs/bird/-/commit/b424442cf8a524365884c277484dde989f9... Will add constant promotion to FI_EQ to handle issue comparing literal '[]' with prefix set variable. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@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."
Hello, Great news! Thanks! On Fri, Mar 4, 2022 at 2:45 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Wed, Feb 23, 2022 at 07:09:33PM +0100, Alexander Zubkov wrote:
On Wed, Feb 23, 2022, 17:43 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Wed, Feb 23, 2022 at 03:46:53PM +0100, Alexander Zubkov wrote:
Updated patch. Again full and incremental.
Thanks, will look at it.
Good. Give me know if some additional work on the patch is needed.
Thanks, merged:
https://gitlab.nic.cz/labs/bird/-/commit/931c6ff51a312f005468f195cc2d27061d3...
I removed the explicit conversion in val_compare(), moved as_path_filter() changes to a separate patch and changed it to use struct f_val (similar functions for *lists also use that).
Already added typechecks for function call, which fixes issue when '[]' is passed as prefix set arg:
https://gitlab.nic.cz/labs/bird/-/commit/b424442cf8a524365884c277484dde989f9...
Will add constant promotion to FI_EQ to handle issue comparing literal '[]' with prefix set variable.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
Hi, So as we have finished the empty prefix set feature, can we continue with my other suggestions? :) On Mon, Feb 14, 2022 at 9:47 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Feb 10, 2022 at 02:09:59PM +0100, Alexander Zubkov wrote:
Hi,
Let me remind about this once again.
Hi
Sorry for ignoring your previous mail, i needed to focus on finishing things for 2.0.9, now i can continue with your patches.
1) UDP logging - I have no principal issue with this, will review and merge.
2) Keeping proto state during reconfiguration - I am ok with 'configure keep' patch, will preview and merge. I do not want 'keep state' per-protocol option. Perhaps also some 'configure xyz' for regular configure and some global option how 'configure' without arguments should behave for people who would like 'configure keep' to be the default behavior.
3) prepend_times() - I am a bit ambivalent about that. I see usefulness of that. I do not like the name. It can be implemented as optional argument to prepend(), but not sure if that is better. Will think about that. Also, considering user mistakes like [*], we perhaps should limit multiplier by say 16.
[*] https://bgpmon.net/long-as-paths-causing-commotion/
4) Empty prefix set - Too hacky and inconsistent syntax. Note that there are also non-prefix sets. We already have ugly hacks for empty community lists, we want to avoid yet another way to define empty structures.
Unfortunately there is no easy way to define literals that could be used for multiple data types. We already have A.B.C.D, that could be both IP address and router ID, and it is super annoying in the code. We would like to have some more generic solution, we will think about that.
5) Reduce/reduce fix - Will look at this.
On Tue, Jan 11, 2022 at 1:10 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Let me ping this. I would like to receive a feedback on this (and some other my recent) messages.
On Sat, Jan 1, 2022 at 9:45 PM Alexander Zubkov <green@qrator.net> wrote:
Hi,
I propose to add some constant for empty prefix set, for example (as in the patch): define pfx_empty = [ /empty/ ];
It is useful when we define a named prefix list, which we can use somewhere in filters, but this prefix list is generated, for example, and we may need to make it empty. But there is no currently means to create an empty prefix set, as I understand.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@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."
participants (2)
-
Alexander Zubkov -
Ondrej Zajicek