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."