[PATCH] adding custom options in radv protocol, strict ipv6 regex
Hi, Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc. This idea is inspired by presentation by Ondřej Caletka at CSNOG, in which he noticed the lack of either PREF64 option or possibility to add custom options in various software. Attached patch [radv-custom.patch] makes it possible to define such options with the syntax: other type <num> <bytestring> I can also prepare a patch for documentation if it is to be merged. But it does solve the question with PREF64 completely. Because currently bytestring minimal length is 16 bytes, but for PREF64 we need to provide a 14-byte blob. And for minimal RA option, it have to be as short as 6 bytes. So another patch [bytestring-short.patch] allows bytestrings to be as short as 6 bytes, when colon-notation is used: "01:02:03:04:05:06". And I kept the minimum size of 16 bytes without colons, because it can conflict with some symbol names. The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those). It is also not possible to define a 8-byte bytestring, because it conflicts with IPv6 address, but we are introducing short bytestrings for RA here, and 8-byte bytestrings are not strictly required for that, because possible option sizes are 8, 16, ... with payloads 2 bytes less: 6, 14, ... So if one needs a 8-byte payload, he can easily pad it with extra zeroes ":00" with the same on-the-wire result. But still, this gives one more reason for an additional syntax for bytestrings. There also was possbility to explicitly allow only 6, 7, 9 and greater lengths of bytestrings. Or to move IPv6 regex definition before bytestring definition to make it more preferrable. I choose the second variant, but IPv6 regex is too loose now and match many things far from IPv6 notation. So I decided to provide a more strict regex definition for IPv6 addresses. Of course it is ugly, but I think it could be helpful anyway, because it does not conflict with other similar lexems, including MAC addresses. As a downside for this, in case of some typo in IPv6 address, there will not be a meaningful error about bad IPv6 address. So I added an additional regex there, to catch lexems with hex digits and at least 2 colons to show more meaningful error for that. But I'm not sure about it. If those changes for bytestrings are OK too, I also can prepare further patch for documentation. Have a nice day, Alexander Zubkov
Hello! This looks like a clever solution for such a problem. Thank you for the patch! Regarding the bytestring syntax, what about adding some syntax like hex(deadbeef12345678) or even base64(...) where the user could write byte blob of any length? Maria On 6/12/23 13:08, Alexander Zubkov via Bird-users wrote:
Hi,
Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc.
This idea is inspired by presentation by Ondřej Caletka at CSNOG, in which he noticed the lack of either PREF64 option or possibility to add custom options in various software.
Attached patch [radv-custom.patch] makes it possible to define such options with the syntax:
other type <num> <bytestring>
I can also prepare a patch for documentation if it is to be merged.
But it does solve the question with PREF64 completely. Because currently bytestring minimal length is 16 bytes, but for PREF64 we need to provide a 14-byte blob. And for minimal RA option, it have to be as short as 6 bytes.
So another patch [bytestring-short.patch] allows bytestrings to be as short as 6 bytes, when colon-notation is used: "01:02:03:04:05:06". And I kept the minimum size of 16 bytes without colons, because it can conflict with some symbol names.
The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those).
It is also not possible to define a 8-byte bytestring, because it conflicts with IPv6 address, but we are introducing short bytestrings for RA here, and 8-byte bytestrings are not strictly required for that, because possible option sizes are 8, 16, ... with payloads 2 bytes less: 6, 14, ... So if one needs a 8-byte payload, he can easily pad it with extra zeroes ":00" with the same on-the-wire result. But still, this gives one more reason for an additional syntax for bytestrings.
There also was possbility to explicitly allow only 6, 7, 9 and greater lengths of bytestrings. Or to move IPv6 regex definition before bytestring definition to make it more preferrable. I choose the second variant, but IPv6 regex is too loose now and match many things far from IPv6 notation. So I decided to provide a more strict regex definition for IPv6 addresses. Of course it is ugly, but I think it could be helpful anyway, because it does not conflict with other similar lexems, including MAC addresses.
As a downside for this, in case of some typo in IPv6 address, there will not be a meaningful error about bad IPv6 address. So I added an additional regex there, to catch lexems with hex digits and at least 2 colons to show more meaningful error for that. But I'm not sure about it.
If those changes for bytestrings are OK too, I also can prepare further patch for documentation.
Have a nice day, Alexander Zubkov
-- Maria Matejka | BIRD Team Leader | CZ.NIC, z.s.p.o.
Hello, Maria! You suggestion for blob syntax seems good to me. I think I can try to prepare patches for that. Only one concern is that it could break some current configuration files, if they have functions with such names. Maybe it is better to use some other "brackets" to make it less possible to hit something? hex<deadbeef> or hex[deadbeef] maybe? Or even something exotic like hex|deadbeef| ? Would you like to keep my proposed changes for IPv6 and bytestrings too? Or just introduce the new syntax? On Mon, Jun 12, 2023 at 1:33 PM Maria Matejka <maria.matejka@nic.cz> wrote:
Hello!
This looks like a clever solution for such a problem. Thank you for the patch!
Regarding the bytestring syntax, what about adding some syntax like hex(deadbeef12345678) or even base64(...) where the user could write byte blob of any length?
Maria On 6/12/23 13:08, Alexander Zubkov via Bird-users wrote:
Hi,
Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc.
This idea is inspired by presentation by Ondřej Caletka at CSNOG, in which he noticed the lack of either PREF64 option or possibility to add custom options in various software.
Attached patch [radv-custom.patch] makes it possible to define such options with the syntax:
other type <num> <bytestring>
I can also prepare a patch for documentation if it is to be merged.
But it does solve the question with PREF64 completely. Because currently bytestring minimal length is 16 bytes, but for PREF64 we need to provide a 14-byte blob. And for minimal RA option, it have to be as short as 6 bytes.
So another patch [bytestring-short.patch] allows bytestrings to be as short as 6 bytes, when colon-notation is used: "01:02:03:04:05:06". And I kept the minimum size of 16 bytes without colons, because it can conflict with some symbol names.
The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those).
It is also not possible to define a 8-byte bytestring, because it conflicts with IPv6 address, but we are introducing short bytestrings for RA here, and 8-byte bytestrings are not strictly required for that, because possible option sizes are 8, 16, ... with payloads 2 bytes less: 6, 14, ... So if one needs a 8-byte payload, he can easily pad it with extra zeroes ":00" with the same on-the-wire result. But still, this gives one more reason for an additional syntax for bytestrings.
There also was possbility to explicitly allow only 6, 7, 9 and greater lengths of bytestrings. Or to move IPv6 regex definition before bytestring definition to make it more preferrable. I choose the second variant, but IPv6 regex is too loose now and match many things far from IPv6 notation. So I decided to provide a more strict regex definition for IPv6 addresses. Of course it is ugly, but I think it could be helpful anyway, because it does not conflict with other similar lexems, including MAC addresses.
As a downside for this, in case of some typo in IPv6 address, there will not be a meaningful error about bad IPv6 address. So I added an additional regex there, to catch lexems with hex digits and at least 2 colons to show more meaningful error for that. But I'm not sure about it.
If those changes for bytestrings are OK too, I also can prepare further patch for documentation.
Have a nice day, Alexander Zubkov
-- Maria Matejka | BIRD Team Leader | CZ.NIC, z.s.p.o.
Hello! I think using hex() and base64() with adding these two tokens to the "kw_sym:" non-terminal. This way, no current config should break. Also it may be handy for both hex() and base64() to accept any number of whitespace inside the argument, to enable configs like hex(de ad be ef) or other direct pastes from hexdumps or base64 encoders. We could then e.g. add the SSH keys for RPKI inline to the config file. I'd prefer keeping the IPv6 and bytestrings as they are now, possibly even deprecating the current bytestring format in future. (Weak opinion on this.) Maria On 6/12/23 14:24, Alexander Zubkov wrote:
Hello, Maria!
You suggestion for blob syntax seems good to me. I think I can try to prepare patches for that. Only one concern is that it could break some current configuration files, if they have functions with such names. Maybe it is better to use some other "brackets" to make it less possible to hit something? hex<deadbeef> or hex[deadbeef] maybe? Or even something exotic like hex|deadbeef| ?
Would you like to keep my proposed changes for IPv6 and bytestrings too? Or just introduce the new syntax?
On Mon, Jun 12, 2023 at 1:33 PM Maria Matejka <maria.matejka@nic.cz> wrote:
Hello!
This looks like a clever solution for such a problem. Thank you for the patch!
Regarding the bytestring syntax, what about adding some syntax like hex(deadbeef12345678) or even base64(...) where the user could write byte blob of any length?
Maria
On 6/12/23 13:08, Alexander Zubkov via Bird-users wrote:
Hi,
Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc.
This idea is inspired by presentation by Ondřej Caletka at CSNOG, in which he noticed the lack of either PREF64 option or possibility to add custom options in various software.
Attached patch [radv-custom.patch] makes it possible to define such options with the syntax:
other type <num> <bytestring>
I can also prepare a patch for documentation if it is to be merged.
But it does solve the question with PREF64 completely. Because currently bytestring minimal length is 16 bytes, but for PREF64 we need to provide a 14-byte blob. And for minimal RA option, it have to be as short as 6 bytes.
So another patch [bytestring-short.patch] allows bytestrings to be as short as 6 bytes, when colon-notation is used: "01:02:03:04:05:06". And I kept the minimum size of 16 bytes without colons, because it can conflict with some symbol names.
The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those).
It is also not possible to define a 8-byte bytestring, because it conflicts with IPv6 address, but we are introducing short bytestrings for RA here, and 8-byte bytestrings are not strictly required for that, because possible option sizes are 8, 16, ... with payloads 2 bytes less: 6, 14, ... So if one needs a 8-byte payload, he can easily pad it with extra zeroes ":00" with the same on-the-wire result. But still, this gives one more reason for an additional syntax for bytestrings.
There also was possbility to explicitly allow only 6, 7, 9 and greater lengths of bytestrings. Or to move IPv6 regex definition before bytestring definition to make it more preferrable. I choose the second variant, but IPv6 regex is too loose now and match many things far from IPv6 notation. So I decided to provide a more strict regex definition for IPv6 addresses. Of course it is ugly, but I think it could be helpful anyway, because it does not conflict with other similar lexems, including MAC addresses.
As a downside for this, in case of some typo in IPv6 address, there will not be a meaningful error about bad IPv6 address. So I added an additional regex there, to catch lexems with hex digits and at least 2 colons to show more meaningful error for that. But I'm not sure about it.
If those changes for bytestrings are OK too, I also can prepare further patch for documentation.
Have a nice day, Alexander Zubkov
-- Maria Matejka | BIRD Team Leader | CZ.NIC, z.s.p.o.
-- Maria Matejka | BIRD Team Leader | CZ.NIC, z.s.p.o.
On Mon, Jun 12, 2023 at 02:40:42PM +0200, Maria Matejka via Bird-users wrote:
Hello!
I think using hex() and base64() with adding these two tokens to the "kw_sym:" non-terminal. This way, no current config should break.
I would prefer hex:XX:YY:... to hex(XX:YY:...) to emphasize it is lexer and not parser thing and avoid special cases. -- 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."
We can simply change the lexer state externally from the parser as soon as the hex( prefix is seen, and provide the result directly from the lexer. This way, we can allow all the syntaxes like hex(de-ad-be-ef), hex(de:ad:be:ef), hex(de ad be ef) or even hex(dea:db-eef) just by ignoring nonalnum characters altogether. Here I'd strongly prefer nicer user experience over setting the syntax to best fit our needs. I don't think any user really cares about the lexer/parser difference. Maria On 12 June 2023 15:07:16 CEST, Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 12, 2023 at 02:40:42PM +0200, Maria Matejka via Bird-users wrote:
Hello!
I think using hex() and base64() with adding these two tokens to the "kw_sym:" non-terminal. This way, no current config should break.
I would prefer hex:XX:YY:... to hex(XX:YY:...) to emphasize it is lexer and not parser thing and avoid special cases.
-- 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."
-- Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.
On Mon, Jun 12, 2023 at 03:32:20PM +0200, Maria Matejka wrote:
We can simply change the lexer state externally from the parser as soon as the hex( prefix is seen, and provide the result directly from the lexer.
This way, we can allow all the syntaxes like hex(de-ad-be-ef), hex(de:ad:be:ef), hex(de ad be ef) or even hex(dea:db-eef) just by ignoring nonalnum characters altogether.
Here I'd strongly prefer nicer user experience over setting the syntax to best fit our needs.
Which would lead to syntax that is extremely confusing (i.e. hard to intuitively grasp the right mental model just from meeting examples), so i think hex(...) variant is also worse from user experience point. As a user, i always hated unexpected special cases in syntax, even if they might be expedient in some cases.
I don't think any user really cares about the lexer/parser difference.
People came with some preconceptions based on knowledge of syntax of other programming / configuration (and also regular) languages, so they have (either explicit or intuitive) concept of elementary / compound statement. If there is someting that looks like existing compound statement, but is in fact something completely different, it is confusing. -- 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."
Some additional ideas for decorating binary strings so that they do not resemble other statements: @hex(...) bin:hex(...) BTW, if we put a string literal inside the brackets, we can mimic a function call without dirty lexer/parser hacks: hex("...") Or maybe you have already agreed on something? On Mon, Jun 12, 2023 at 4:02 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 12, 2023 at 03:32:20PM +0200, Maria Matejka wrote:
We can simply change the lexer state externally from the parser as soon as the hex( prefix is seen, and provide the result directly from the lexer.
This way, we can allow all the syntaxes like hex(de-ad-be-ef), hex(de:ad:be:ef), hex(de ad be ef) or even hex(dea:db-eef) just by ignoring nonalnum characters altogether.
Here I'd strongly prefer nicer user experience over setting the syntax to best fit our needs.
Which would lead to syntax that is extremely confusing (i.e. hard to intuitively grasp the right mental model just from meeting examples), so i think hex(...) variant is also worse from user experience point. As a user, i always hated unexpected special cases in syntax, even if they might be expedient in some cases.
I don't think any user really cares about the lexer/parser difference.
People came with some preconceptions based on knowledge of syntax of other programming / configuration (and also regular) languages, so they have (either explicit or intuitive) concept of elementary / compound statement. If there is someting that looks like existing compound statement, but is in fact something completely different, it is confusing.
-- 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."
Well, we haven't agreed yet. We're gonna meet tomorrow so there may be a discussion about this in person and then we'll come with some agreement. Anyway, thank you for all the input! Maria On 12 June 2023 17:55:34 CEST, Alexander Zubkov <green@qrator.net> wrote:
Some additional ideas for decorating binary strings so that they do not resemble other statements: @hex(...) bin:hex(...)
BTW, if we put a string literal inside the brackets, we can mimic a function call without dirty lexer/parser hacks: hex("...")
Or maybe you have already agreed on something?
On Mon, Jun 12, 2023 at 4:02 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 12, 2023 at 03:32:20PM +0200, Maria Matejka wrote:
We can simply change the lexer state externally from the parser as soon as the hex( prefix is seen, and provide the result directly from the lexer.
This way, we can allow all the syntaxes like hex(de-ad-be-ef), hex(de:ad:be:ef), hex(de ad be ef) or even hex(dea:db-eef) just by ignoring nonalnum characters altogether.
Here I'd strongly prefer nicer user experience over setting the syntax to best fit our needs.
Which would lead to syntax that is extremely confusing (i.e. hard to intuitively grasp the right mental model just from meeting examples), so i think hex(...) variant is also worse from user experience point. As a user, i always hated unexpected special cases in syntax, even if they might be expedient in some cases.
I don't think any user really cares about the lexer/parser difference.
People came with some preconceptions based on knowledge of syntax of other programming / configuration (and also regular) languages, so they have (either explicit or intuitive) concept of elementary / compound statement. If there is someting that looks like existing compound statement, but is in fact something completely different, it is confusing.
-- 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."
-- Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.
On Mon, Jun 12, 2023 at 05:55:34PM +0200, Alexander Zubkov wrote:
BTW, if we put a string literal inside the brackets, we can mimic a function call without dirty lexer/parser hacks: hex("...")
Or maybe you have already agreed on something?
Hi We agreed on keeping existing format for suffiently long hex bytestrings, using hex: prefix for bytestrings of any length, and adding hex("...") / base64("...") as ordinary expressions. -- 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, Jun 13, 2023, 16:07 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 12, 2023 at 05:55:34PM +0200, Alexander Zubkov wrote:
BTW, if we put a string literal inside the brackets, we can mimic a function call without dirty lexer/parser hacks: hex("...")
Or maybe you have already agreed on something?
Hi
We agreed on keeping existing format for suffiently long hex bytestrings, using hex: prefix for bytestrings of any length, and adding hex("...") / base64("...") as ordinary expressions.
Hi, So full house. :) I can try to implement it. Do you need a hand?
-- 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, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek <santiago@crfreenet.org> wrote:
We agreed on keeping existing format for suffiently long hex bytestrings, using hex: prefix for bytestrings of any length, and adding hex("...") / base64("...") as ordinary expressions.
Hi,
So full house. :) I can try to implement it. Do you need a hand?
If you implement hex: prefix, we could easily merge that. The hex("...") / base64("...") is not priority and would require some nontrivial changes to be done properly. -- 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, Please look at these patches: bytestring-hex-prefix.patch - syntax with "hex:" prefix I allowed mixed colons with no-divider there, so hex:12:345678:90 is allowed. As there is a distinguishing prefix here, this should not be a problem. Empty bytestrings are allowed too: "hex:" bytestring-hex-function.patch - function-like hex("...") syntax (on top of the other patch) It wasn't too complex, but you might have wanted to do it some other way. I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings. I parse string in two steps. First to know the length of a binary output. Current hex realization does the same anyway. I made a separate function for it to reuse it for "classic" bytestrings and for function-like syntax. It ignores all non-alphanumeric symbols and requires that each byte symbols go in pairs without delimiters. I added a HEX keyword and added it to kw_sym, but this solution does not allow to mix "hex" symbol and hex("...") bytestring in the same config. There probably was a mistake in nest/config.Y with missing "|" here: "kw_sym: MIN MAX ;" There is a "TODO" in the heading of strtobin.c, don't know what is your policy regarding that. The code is still based on the current BYTESTRING parser. I also enabled TEXT literals to be multiline. Didn't know it was not allowed already, so I think it should not break things, but allows to be more flexible with binary strings. On Tue, Jun 13, 2023 at 6:37 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek <santiago@crfreenet.org> wrote:
We agreed on keeping existing format for suffiently long hex bytestrings, using hex: prefix for bytestrings of any length, and adding hex("...") / base64("...") as ordinary expressions.
Hi,
So full house. :) I can try to implement it. Do you need a hand?
If you implement hex: prefix, we could easily merge that. The hex("...") / base64("...") is not priority and would require some nontrivial changes to be done properly.
-- 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, While waiting for the fate of the previous patches, I was thinking about that thing about using keywords as symbols. So here is another longread. :) Now it is not possible to mix a keyword and a keyword as a symbol together. Here is what I mean. With current master bird if I use config: protocol device {} template bgp { local role peer; } function role() { int role = 1; return role; } It parses ok, because "role" keyword is used before it is converted into a symbol. But if it appears as a symbol before: protocol device {} function role() { int role = 1; return role; } template bgp { local role peer; } It returns an error: bird: t5.conf:3:22 IP address constant expected Because "role" is converted to a symbol and it is not possible to use it as a keyword anymore. I thought if something can be done about that and I probably have a solution. See the attached patch. It maybe not the best design, just to show the idea. I change the order of detecting symbol and keyword in the lexer, so it always returns a keyword for a keyword, and it is converted into a symbol only in the parser when it is used as a symbol. As we can hit such symbol keyword many times, I check if it has already been defined and create a new one if needed. Then I replace mentions of CF_SYM_KNOWN with symbol_known, which can be CF_SYM_KNOWN or kw_sym. So, with this patch applied, both configs are successfully parsed and work well. Also, I think that the current realization in bird relies on the fact that lexer would not have symbols parsed in advance, i.e. that further mentions of the keyword should return CF_SYM_KNOWN. But if it is not the case and the lexer parses forward for some reason (before the parser creates the symbol for the keyword) it would not return CF_SYM_KNOWN. I don't know the internals of the lexer and parser much, maybe it is impossible situation (parser does not backtrack, etc.). And there is no need to worry here. Some additional notes. My previous remark regarding the missing "|" in "kw_sym: MIN MAX" seems to be correct, because current bird master fails on such config: protocol device {} function min() { return 0; } With the error: bird: t4.conf:2:13 syntax error, unexpected '(', expecting MAX And also there seems to be a bug with recently added syntax for variable definitions. When they are defined without initializing. This config is successfully loaded: protocol device {} function role() { int role; role = 1; return role; } But if I try to call "eval role()" from cli, I get an error: "runtime error", and the daemon logs this: bird: filters, line 2: Argument 1 of VAR_INIT must be of type int, got type void So it looks like it tries to initialize it with the void value and fails to do it. I haven't tracked the source of this bug. On Wed, Jun 14, 2023 at 12:40 AM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Please look at these patches:
bytestring-hex-prefix.patch - syntax with "hex:" prefix I allowed mixed colons with no-divider there, so hex:12:345678:90 is allowed. As there is a distinguishing prefix here, this should not be a problem. Empty bytestrings are allowed too: "hex:"
bytestring-hex-function.patch - function-like hex("...") syntax (on top of the other patch) It wasn't too complex, but you might have wanted to do it some other way. I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings. I parse string in two steps. First to know the length of a binary output. Current hex realization does the same anyway. I made a separate function for it to reuse it for "classic" bytestrings and for function-like syntax. It ignores all non-alphanumeric symbols and requires that each byte symbols go in pairs without delimiters. I added a HEX keyword and added it to kw_sym, but this solution does not allow to mix "hex" symbol and hex("...") bytestring in the same config. There probably was a mistake in nest/config.Y with missing "|" here: "kw_sym: MIN MAX ;" There is a "TODO" in the heading of strtobin.c, don't know what is your policy regarding that. The code is still based on the current BYTESTRING parser. I also enabled TEXT literals to be multiline. Didn't know it was not allowed already, so I think it should not break things, but allows to be more flexible with binary strings.
On Tue, Jun 13, 2023 at 6:37 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek <santiago@crfreenet.org> wrote:
We agreed on keeping existing format for suffiently long hex bytestrings, using hex: prefix for bytestrings of any length, and adding hex("...") / base64("...") as ordinary expressions.
Hi,
So full house. :) I can try to implement it. Do you need a hand?
If you implement hex: prefix, we could easily merge that. The hex("...") / base64("...") is not priority and would require some nontrivial changes to be done properly.
-- 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, Please give some feedback. On Thu, Jun 15, 2023 at 3:57 AM Alexander Zubkov <green@qrator.net> wrote:
Hi,
While waiting for the fate of the previous patches, I was thinking about that thing about using keywords as symbols. So here is another longread. :)
Now it is not possible to mix a keyword and a keyword as a symbol together. Here is what I mean. With current master bird if I use config:
protocol device {} template bgp { local role peer; } function role() { int role = 1; return role; }
It parses ok, because "role" keyword is used before it is converted into a symbol. But if it appears as a symbol before:
protocol device {} function role() { int role = 1; return role; } template bgp { local role peer; }
It returns an error:
bird: t5.conf:3:22 IP address constant expected
Because "role" is converted to a symbol and it is not possible to use it as a keyword anymore. I thought if something can be done about that and I probably have a solution. See the attached patch. It maybe not the best design, just to show the idea. I change the order of detecting symbol and keyword in the lexer, so it always returns a keyword for a keyword, and it is converted into a symbol only in the parser when it is used as a symbol. As we can hit such symbol keyword many times, I check if it has already been defined and create a new one if needed. Then I replace mentions of CF_SYM_KNOWN with symbol_known, which can be CF_SYM_KNOWN or kw_sym. So, with this patch applied, both configs are successfully parsed and work well. Also, I think that the current realization in bird relies on the fact that lexer would not have symbols parsed in advance, i.e. that further mentions of the keyword should return CF_SYM_KNOWN. But if it is not the case and the lexer parses forward for some reason (before the parser creates the symbol for the keyword) it would not return CF_SYM_KNOWN. I don't know the internals of the lexer and parser much, maybe it is impossible situation (parser does not backtrack, etc.). And there is no need to worry here.
Some additional notes.
My previous remark regarding the missing "|" in "kw_sym: MIN MAX" seems to be correct, because current bird master fails on such config:
protocol device {} function min() { return 0; }
With the error:
bird: t4.conf:2:13 syntax error, unexpected '(', expecting MAX
And also there seems to be a bug with recently added syntax for variable definitions. When they are defined without initializing. This config is successfully loaded:
protocol device {} function role() { int role; role = 1; return role; }
But if I try to call "eval role()" from cli, I get an error: "runtime error", and the daemon logs this:
bird: filters, line 2: Argument 1 of VAR_INIT must be of type int, got type void
So it looks like it tries to initialize it with the void value and fails to do it. I haven't tracked the source of this bug.
On Wed, Jun 14, 2023 at 12:40 AM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Please look at these patches:
bytestring-hex-prefix.patch - syntax with "hex:" prefix I allowed mixed colons with no-divider there, so hex:12:345678:90 is allowed. As there is a distinguishing prefix here, this should not be a problem. Empty bytestrings are allowed too: "hex:"
bytestring-hex-function.patch - function-like hex("...") syntax (on top of the other patch) It wasn't too complex, but you might have wanted to do it some other way. I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings. I parse string in two steps. First to know the length of a binary output. Current hex realization does the same anyway. I made a separate function for it to reuse it for "classic" bytestrings and for function-like syntax. It ignores all non-alphanumeric symbols and requires that each byte symbols go in pairs without delimiters. I added a HEX keyword and added it to kw_sym, but this solution does not allow to mix "hex" symbol and hex("...") bytestring in the same config. There probably was a mistake in nest/config.Y with missing "|" here: "kw_sym: MIN MAX ;" There is a "TODO" in the heading of strtobin.c, don't know what is your policy regarding that. The code is still based on the current BYTESTRING parser. I also enabled TEXT literals to be multiline. Didn't know it was not allowed already, so I think it should not break things, but allows to be more flexible with binary strings.
On Tue, Jun 13, 2023 at 6:37 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek <santiago@crfreenet.org> wrote:
We agreed on keeping existing format for suffiently long hex bytestrings, using hex: prefix for bytestrings of any length, and adding hex("...") / base64("...") as ordinary expressions.
Hi,
So full house. :) I can try to implement it. Do you need a hand?
If you implement hex: prefix, we could easily merge that. The hex("...") / base64("...") is not priority and would require some nontrivial changes to be done properly.
-- 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 Thu, Jun 22, 2023 at 09:04:11PM +0200, Alexander Zubkov wrote:
Hi,
Please give some feedback.
Hi Sorry for letting you wait, will look at it tomorrow. -- 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 Thu, Jun 15, 2023 at 03:57:10AM +0200, Alexander Zubkov wrote:
Hi,
While waiting for the fate of the previous patches, I was thinking about that thing about using keywords as symbols. So here is another longread. :)
Now it is not possible to mix a keyword and a keyword as a symbol together. Here is what I mean. With current master bird if I use config: ... protocol device {} function role() { int role = 1; return role; } template bgp { local role peer; }
Hi You are right, but note that keywords collide just with top-level symbols (e.g. protocols, tables, functions, ...), while scoped symbols do not affect it, e.g, this is okay: function xyz() { int role = 1; return role; } template bgp { local role peer; } While it is inconvenient, it preserves backward compatibility (compared to the previous approach, where keywords always dominate symbols), as a configuration valid for an older version of BIRD is valid in a newer version of BIRD that adds new keywords (the configuration may contain such names used as symbols, but not as keywords).
Because "role" is converted to a symbol and it is not possible to use it as a keyword anymore. I thought if something can be done about that and I probably have a solution. See the attached patch. It maybe not the best design, just to show the idea. I change the order of detecting symbol and keyword in the lexer, so it always returns a keyword for a keyword, and it is converted into a symbol only in the parser when it is used as a symbol. As we can hit such symbol keyword many times, I check if it has already been defined and create a new one if needed. Then I replace mentions of CF_SYM_KNOWN with symbol_known, which can be CF_SYM_KNOWN or kw_sym. So, with this patch applied, both configs are successfully parsed and work well.
We thought about this approach, but it has issues with conflicts in our rather irregular grammar. Note that current kw_sym is almost empty, i originally planned it to contain all keywords (unless explicitly excluded), and i would likely to do that soon. Even with almost empty kw_sym, your patch added 2 grammar conflicts. If we add most symbols to kw_sym, it will cause many conflicts. OTOH, the current approach accepts kw_sym only in 'symbol' [*], which is much scarced in grammar, and therefore causes minimum conflicts. [*] Which is in-fact more like symbol_unknown, but also accepts known symbols for purpose of redefining them in different scope. I would prefer to avoid such changes now, as Maria is doing some rework on symbols and we have some divergence in symbol processing between master and 3.0 branches.
Also, I think that the current realization in bird relies on the fact that lexer would not have symbols parsed in advance, i.e. that further mentions of the keyword should return CF_SYM_KNOWN. But if it is not the case and the lexer parses forward for some reason (before the parser creates the symbol for the keyword) it would not return CF_SYM_KNOWN. I don't know the internals of the lexer and parser much, maybe it is impossible situation (parser does not backtrack, etc.). And there is no need to worry here.
Yes, it is kind of strange, in long-term, it would make more sense to move all symbol processing from lexer to parser.
Some additional notes.
My previous remark regarding the missing "|" in "kw_sym: MIN MAX" seems to be correct, because current bird master fails on such config:
Yes that is true. I just added it ad-hoc in order to have some kw_sym defined in nest/config.Y, so it is not empty when BIRD is compiled without BGP.
But if I try to call "eval role()" from cli, I get an error: "runtime error", and the daemon logs this:
bird: filters, line 2: Argument 1 of VAR_INIT must be of type int, got type void
So it looks like it tries to initialize it with the void value and fails to do it. I haven't tracked the source of this bug.
Will look at this. -- 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! On 6/24/23 15:13, Ondrej Zajicek wrote:
On Thu, Jun 15, 2023 at 03:57:10AM +0200, Alexander Zubkov wrote:
Also, I think that the current realization in bird relies on the fact that lexer would not have symbols parsed in advance, i.e. that further mentions of the keyword should return CF_SYM_KNOWN. But if it is not the case and the lexer parses forward for some reason (before the parser creates the symbol for the keyword) it would not return CF_SYM_KNOWN. I don't know the internals of the lexer and parser much, maybe it is impossible situation (parser does not backtrack, etc.). And there is no need to worry here. Yes, it is kind of strange, in long-term, it would make more sense to move all symbol processing from lexer to parser.
I was moving the symbol processing from parser to lexer several years ago due to Bison limitations. Flex afaik guarantees that it only parses one token at a time. The Bison-Flex boundary is a nasty can of worms and I'm afraid that the best way to get rid of it is to get rid of it altogether. Yet for now, I'd prefer keeping it as is. BTW the new method-call code (branch mq-func-types) depends exactly on the fact that I can manipulate lexer state/context from parser immediately before parsing the following token. Maria -- Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.
Hello! On Sat, Jun 24, 2023 at 3:30 PM Maria Matejka <maria.matejka@nic.cz> wrote:
Hello!
On 6/24/23 15:13, Ondrej Zajicek wrote:
On Thu, Jun 15, 2023 at 03:57:10AM +0200, Alexander Zubkov wrote:
Also, I think that the current realization in bird relies on the fact that lexer would not have symbols parsed in advance, i.e. that further mentions of the keyword should return CF_SYM_KNOWN. But if it is not the case and the lexer parses forward for some reason (before the parser creates the symbol for the keyword) it would not return CF_SYM_KNOWN. I don't know the internals of the lexer and parser much, maybe it is impossible situation (parser does not backtrack, etc.). And there is no need to worry here.
Yes, it is kind of strange, in long-term, it would make more sense to move all symbol processing from lexer to parser.
I was moving the symbol processing from parser to lexer several years ago due to Bison limitations. Flex afaik guarantees that it only parses one token at a time. The Bison-Flex boundary is a nasty can of worms and I'm afraid that the best way to get rid of it is to get rid of it altogether. Yet for now, I'd prefer keeping it as is.
Yes, I see. There are two languages with different structure (config and filter) in the same grammar. And that requires some tradeoffs to be made, at least with its current parsing approach.
BTW the new method-call code (branch mq-func-types) depends exactly on the fact that I can manipulate lexer state/context from parser immediately before parsing the following token.
Maria
-- Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.
On Sat, Jun 24, 2023 at 03:13:11PM +0200, Ondrej Zajicek wrote:
But if I try to call "eval role()" from cli, I get an error: "runtime error", and the daemon logs this:
bird: filters, line 2: Argument 1 of VAR_INIT must be of type int, got type void
So it looks like it tries to initialize it with the void value and fails to do it. I haven't tracked the source of this bug.
Yes, it fails on run-time type check, as VAR_INIT instruction tries to initialize the variable with T_VOID value. This is not an issue with old-style variable definitions, as they are not initialized by an bytecode instruction, they are just memset to zero (i.e. T_VOID) when a stack frame is created during function call. I did not noticed that in tests as all my tests use initialization to an explicit value. Similar issue is here also with old-style variables, which are pre-initialized with T_VOID: function fun() int x; int y; { x = y; return x; } In this case both x and y are initialized to T_VOID, but then the assignment x = y fails in VAR_SET instruction, as it expects int and got void from y. This was (unintentionally) introduced in 2.0.8, with improved typechecking of filters. The example above is a toy example, but there are realistic examples with route attributes (which are void when undefined): int x = bgp_med; # fails with error when bgp_med is undefined while in older BIRD (pre 2.0.8) an equivalent of: int x = bgp_med; if defined(x) then ... worked. The handling of undefined value is inconsistent, and i now i remember that we had this discussion in the past: https://bird.network.cz/pipermail/bird-users/2018-January/011826.html -- 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, Jun 14, 2023 at 12:40:47AM +0200, Alexander Zubkov wrote:
Hi,
Please look at these patches:
bytestring-hex-prefix.patch - syntax with "hex:" prefix I allowed mixed colons with no-divider there, so hex:12:345678:90 is allowed. As there is a distinguishing prefix here, this should not be a problem. Empty bytestrings are allowed too: "hex:"
Hi Merged: https://gitlab.nic.cz/labs/bird/-/commit/65d6a525944faa3f77041419991d77106d8... I have mixed opinion on mixed colons here :-)
bytestring-hex-function.patch - function-like hex("...") syntax (on top of the other patch) It wasn't too complex, but you might have wanted to do it some other way.
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings.
This is an issue with any keyword, so not a big thing.
There probably was a mistake in nest/config.Y with missing "|" here: "kw_sym: MIN MAX ;"
You are right there.
I also enabled TEXT literals to be multiline. Didn't know it was not allowed already, so I think it should not break things, but allows to be more flexible with binary strings.
That is probably right. -- 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 Fri, Jun 23, 2023, 18:30 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Wed, Jun 14, 2023 at 12:40:47AM +0200, Alexander Zubkov wrote:
Hi,
Please look at these patches:
bytestring-hex-prefix.patch - syntax with "hex:" prefix I allowed mixed colons with no-divider there, so hex:12:345678:90 is allowed. As there is a distinguishing prefix here, this should not be a problem. Empty bytestrings are allowed too: "hex:"
Hi
Merged:
https://gitlab.nic.cz/labs/bird/-/commit/65d6a525944faa3f77041419991d77106d8...
I have mixed opinion on mixed colons here :-)
bytestring-hex-function.patch - function-like hex("...") syntax (on top of the other patch) It wasn't too complex, but you might have wanted to do it some other way.
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. For example they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings.
This is an issue with any keyword, so not a big thing.
Yes. By the way what do you think about the patch that allows using keywords and symbols together? Is it viable?
There probably was a mistake in nest/config.Y with missing "|" here: "kw_sym: MIN MAX ;"
You are right there.
I also enabled TEXT literals to be multiline. Didn't know it was not allowed already, so I think it should not break things, but allows to be more flexible with binary strings.
That is probably right.
-- 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 Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote:
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
Yes, that is true. You can try it if you are brave enough to add new f_val type.
I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings.
This is an issue with any keyword, so not a big thing.
Yes. By the way what do you think about the patch that allows using keywords and symbols together? Is it viable?
Answered there. -- 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 Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote:
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
Yes, that is true.
You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions. Some remarks: It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it. Bytestrings for password caused conflicts with strings on being presented as symbol names. Insted of adding something like "string_or_bytestring", I decided to allow bytestring term to be derived from strings too. I also used different approach from "expr", where it evaluates only complex expressions. I decided that handling constants and symbols via evalutation would be easier. So I have special case only for literals. There is separate "term_bs" term, so that it is possible to use "from_hex()" without outer brackets. Again, added a new file in lib dir for bytestrings. And I changed some struct bytestring pointers to const struct bytestring.
I think this should be quite good too, the only problem with it is inability to mix "hex" symbol with hex("...") bytestrings.
This is an issue with any keyword, so not a big thing.
Yes. By the way what do you think about the patch that allows using keywords and symbols together? Is it viable?
Answered there.
-- 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, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote:
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
Yes, that is true.
You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions.
Hi It looks pretty good. First, could you split it to at least four patches? 1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst) 4) change to parser related to f_eval_val(), bytestring nonterminal and so on. Some more comments:
It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on. Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like: static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; } I will give more comments later. -- 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, Jun 26, 2023 at 5:43 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote:
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
Yes, that is true.
You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions.
Hi
It looks pretty good. First, could you split it to at least four patches?
Sure. I'll provide split patches later.
1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst) 4) change to parser related to f_eval_val(), bytestring nonterminal and so on.
Some more comments:
It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on.
Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like:
static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; }
I will give more comments later.
-- 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, Jun 26, 2023 at 5:54 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote:
Yes, the original idea there was to add bytestring as a data type, make hex() a regular (filter) function instead of special function-like syntax, and add equivalent of 'expr' grammar term for other data types.
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
Yes, that is true.
You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions.
Hi
It looks pretty good. First, could you split it to at least four patches?
Sure. I'll provide split patches later.
1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst)
Added patches up to this point. There are also some fixes and modification. For example, I noted that 'bytestring' symbol for the type name conflicts with lexer's BYTESTRING id. So I had to rename lexer's BYTESTRING to BYTETEXT (like it is done for strings). For the following patches it is better to decide the structure of the new *eval* functions.
4) change to parser related to f_eval_val(), bytestring nonterminal and so on.
Some more comments:
It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on.
Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like:
static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; }
I actually tried first to return the struct instead of modifying it by a reference. But for that we need to have "struct f_val" known in filter/filter.h, which is defined in filter/data.h. But that causes some circular dependencies problem. I didn't dig deep into it, but maybe it is possible to solve the conflict in a clean way.
I will give more comments later.
-- 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, Jun 27, 2023 at 2:13 AM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote:
> Yes, the original idea there was to add bytestring as a data type, make > hex() a regular (filter) function instead of special function-like > syntax, and add equivalent of 'expr' grammar term for other data types. >
I see. I think I can look into preparing a patch for that too. But for such variant I would suggest using function names like "from_hex/base64" instead of "hex/base64", or something including bytestring reference: "bs_hex". Because the simple variants could be misleading when used not only in the limited set of scopes. they can be thought of converting to hex/base64 representation too. Or they could collide with "hex" function to convert from string to int, which someone would need to implement in the future.
Yes, that is true.
You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions.
Hi
It looks pretty good. First, could you split it to at least four patches?
Sure. I'll provide split patches later.
1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst)
Added patches up to this point. There are also some fixes and modification. For example, I noted that 'bytestring' symbol for the type name conflicts with lexer's BYTESTRING id. So I had to rename lexer's BYTESTRING to BYTETEXT (like it is done for strings). For the following patches it is better to decide the structure of the new *eval* functions.
4) change to parser related to f_eval_val(), bytestring nonterminal and so on.
Final patches to modify current f_eval_int() to generic approach. And for nonterminal bytestring. Again, waiting for comments/suggestions. Also feel free, of course, to fix naming/etc when applying. Next, I will be able to move forward with patches to the documentation.
Some more comments:
It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on.
Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like:
static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; }
I actually tried first to return the struct instead of modifying it by a reference. But for that we need to have "struct f_val" known in filter/filter.h, which is defined in filter/data.h. But that causes some circular dependencies problem. I didn't dig deep into it, but maybe it is possible to solve the conflict in a clean way.
Looked at it again. It seems safe to include filter/data.h into filter/filter.h. I probably had problems when trying to incude it somewhere else, until it all finalized into filter.h
I will give more comments later.
-- 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."
Patch for RAdv documentation for a new custom option. I was also thinking about the new bytestring type. I needed tho change BYTESTRING -> BYTETEXT to avoid collision. But probably the better variant would be to name the new type for example "binary", it might sound better. What do you think? As far as I see, the name "bytestring" has not been used yet outside of the code. Also found a small issue with my patches in "conf/confbase.Y" when compiling with relatively old gcc. It complains on the value definitions inside the switch statement: case T_STRING: int len = strlen(val.val.s); struct bytestring *bs = cfg_allocz(sizeof(*bs) + len); I can prepare a new set of patches or you can fix it ad-hoc. On Thu, Jun 29, 2023 at 1:59 PM Alexander Zubkov <green@qrator.net> wrote:
On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote: > > Yes, the original idea there was to add bytestring as a data type, make > > hex() a regular (filter) function instead of special function-like > > syntax, and add equivalent of 'expr' grammar term for other data types. > > > > I see. I think I can look into preparing a patch for that too. > But for such variant I would suggest using function names like > "from_hex/base64" instead of "hex/base64", or something including > bytestring reference: "bs_hex". Because the simple variants could be > misleading when used not only in the limited set of scopes. > they can be thought of converting to hex/base64 representation too. Or they > could collide with "hex" function to convert from string to int, which > someone would need to implement in the future.
Yes, that is true.
You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions.
Hi
It looks pretty good. First, could you split it to at least four patches?
Sure. I'll provide split patches later.
1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst)
Added patches up to this point. There are also some fixes and modification. For example, I noted that 'bytestring' symbol for the type name conflicts with lexer's BYTESTRING id. So I had to rename lexer's BYTESTRING to BYTETEXT (like it is done for strings). For the following patches it is better to decide the structure of the new *eval* functions.
4) change to parser related to f_eval_val(), bytestring nonterminal and so on.
Final patches to modify current f_eval_int() to generic approach. And for nonterminal bytestring. Again, waiting for comments/suggestions. Also feel free, of course, to fix naming/etc when applying. Next, I will be able to move forward with patches to the documentation.
Some more comments:
It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on.
Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like:
static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; }
I actually tried first to return the struct instead of modifying it by a reference. But for that we need to have "struct f_val" known in filter/filter.h, which is defined in filter/data.h. But that causes some circular dependencies problem. I didn't dig deep into it, but maybe it is possible to solve the conflict in a clean way.
Looked at it again. It seems safe to include filter/data.h into filter/filter.h. I probably had problems when trying to incude it somewhere else, until it all finalized into filter.h
I will give more comments later.
-- 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, And the final patch for the bytestring documentation. Also slightly modified radv documentation patch - added a semicolon in the end of the example. I actually would prefer the "binary" name for the type more than "bytestring". Or maybe you have something else on your mind. So if you would also like not to name it "bytestring", I can alter the pathes for it. On Fri, Jun 30, 2023 at 1:21 AM Alexander Zubkov <green@qrator.net> wrote:
Patch for RAdv documentation for a new custom option.
I was also thinking about the new bytestring type. I needed tho change BYTESTRING -> BYTETEXT to avoid collision. But probably the better variant would be to name the new type for example "binary", it might sound better. What do you think? As far as I see, the name "bytestring" has not been used yet outside of the code.
Also found a small issue with my patches in "conf/confbase.Y" when compiling with relatively old gcc. It complains on the value definitions inside the switch statement:
case T_STRING: int len = strlen(val.val.s); struct bytestring *bs = cfg_allocz(sizeof(*bs) + len);
I can prepare a new set of patches or you can fix it ad-hoc.
On Thu, Jun 29, 2023 at 1:59 PM Alexander Zubkov <green@qrator.net> wrote:
On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote: > > On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote: > > > Yes, the original idea there was to add bytestring as a data type, make > > > hex() a regular (filter) function instead of special function-like > > > syntax, and add equivalent of 'expr' grammar term for other data types. > > > > > > > I see. I think I can look into preparing a patch for that too. > > But for such variant I would suggest using function names like > > "from_hex/base64" instead of "hex/base64", or something including > > bytestring reference: "bs_hex". Because the simple variants could be > > misleading when used not only in the limited set of scopes. > > they can be thought of converting to hex/base64 representation too. Or they > > could collide with "hex" function to convert from string to int, which > > someone would need to implement in the future. > > Yes, that is true. > > You can try it if you are brave enough to add new f_val type.
Take a look at the patch, please. Waiting for the critics and improvement suggestions.
Hi
It looks pretty good. First, could you split it to at least four patches?
Sure. I'll provide split patches later.
1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst)
Added patches up to this point. There are also some fixes and modification. For example, I noted that 'bytestring' symbol for the type name conflicts with lexer's BYTESTRING id. So I had to rename lexer's BYTESTRING to BYTETEXT (like it is done for strings). For the following patches it is better to decide the structure of the new *eval* functions.
4) change to parser related to f_eval_val(), bytestring nonterminal and so on.
Final patches to modify current f_eval_int() to generic approach. And for nonterminal bytestring. Again, waiting for comments/suggestions. Also feel free, of course, to fix naming/etc when applying. Next, I will be able to move forward with patches to the documentation.
Some more comments:
It was needed to add another function like f_eval_int(), so I decided to do some more generic approach and replaced all occurences of f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on.
Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like:
static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; }
I actually tried first to return the struct instead of modifying it by a reference. But for that we need to have "struct f_val" known in filter/filter.h, which is defined in filter/data.h. But that causes some circular dependencies problem. I didn't dig deep into it, but maybe it is possible to solve the conflict in a clean way.
Looked at it again. It seems safe to include filter/data.h into filter/filter.h. I probably had problems when trying to incude it somewhere else, until it all finalized into filter.h
I will give more comments later.
-- 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, Have you had a chance to look at all this? On Fri, Jul 7, 2023 at 12:55 AM Alexander Zubkov <green@qrator.net> wrote:
Hi,
And the final patch for the bytestring documentation. Also slightly modified radv documentation patch - added a semicolon in the end of the example. I actually would prefer the "binary" name for the type more than "bytestring". Or maybe you have something else on your mind. So if you would also like not to name it "bytestring", I can alter the pathes for it.
On Fri, Jun 30, 2023 at 1:21 AM Alexander Zubkov <green@qrator.net> wrote:
Patch for RAdv documentation for a new custom option.
I was also thinking about the new bytestring type. I needed tho change BYTESTRING -> BYTETEXT to avoid collision. But probably the better variant would be to name the new type for example "binary", it might sound better. What do you think? As far as I see, the name "bytestring" has not been used yet outside of the code.
Also found a small issue with my patches in "conf/confbase.Y" when compiling with relatively old gcc. It complains on the value definitions inside the switch statement:
case T_STRING: int len = strlen(val.val.s); struct bytestring *bs = cfg_allocz(sizeof(*bs) + len);
I can prepare a new set of patches or you can fix it ad-hoc.
On Thu, Jun 29, 2023 at 1:59 PM Alexander Zubkov <green@qrator.net> wrote:
On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov <green@qrator.net> wrote:
On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote: > On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek <santiago@crfreenet.org> wrote: > > > > On Sat, Jun 24, 2023 at 02:20:03AM +0200, Alexander Zubkov wrote: > > > > Yes, the original idea there was to add bytestring as a data type, make > > > > hex() a regular (filter) function instead of special function-like > > > > syntax, and add equivalent of 'expr' grammar term for other data types. > > > > > > > > > > I see. I think I can look into preparing a patch for that too. > > > But for such variant I would suggest using function names like > > > "from_hex/base64" instead of "hex/base64", or something including > > > bytestring reference: "bs_hex". Because the simple variants could be > > > misleading when used not only in the limited set of scopes. > > > they can be thought of converting to hex/base64 representation too. Or they > > > could collide with "hex" function to convert from string to int, which > > > someone would need to implement in the future. > > > > Yes, that is true. > > > > You can try it if you are brave enough to add new f_val type. > > Take a look at the patch, please. Waiting for the critics and > improvement suggestions.
Hi
It looks pretty good. First, could you split it to at least four patches?
Sure. I'll provide split patches later.
1) unrelated changes, like the newline-in-string-constant 2) preparatory changes (functions in lib/bytestring.c, change to BYTESTRING lexer) 3) adding bytestring type to filter code (including FI_FROM_HEX inst)
Added patches up to this point. There are also some fixes and modification. For example, I noted that 'bytestring' symbol for the type name conflicts with lexer's BYTESTRING id. So I had to rename lexer's BYTESTRING to BYTETEXT (like it is done for strings). For the following patches it is better to decide the structure of the new *eval* functions.
4) change to parser related to f_eval_val(), bytestring nonterminal and so on.
Final patches to modify current f_eval_int() to generic approach. And for nonterminal bytestring. Again, waiting for comments/suggestions. Also feel free, of course, to fix naming/etc when applying. Next, I will be able to move forward with patches to the documentation.
Some more comments:
> It was needed to add another function like f_eval_int(), so I decided > to do some more generic approach and replaced all occurences of > f_eval_int() with it.
That is good approach, although it would be probably better to call this function like cf_eval(), associated macro as cf_eval_val, and keep some inline functions like cf_eval_int(), cf_eval_bs() and so on.
Or perhaps cf_eval() could return f_val as return value, and have shorthand functions like:
static inline cf_eval_int(..) { return cf_eval(.., T_INT).i; }
I actually tried first to return the struct instead of modifying it by a reference. But for that we need to have "struct f_val" known in filter/filter.h, which is defined in filter/data.h. But that causes some circular dependencies problem. I didn't dig deep into it, but maybe it is possible to solve the conflict in a clean way.
Looked at it again. It seems safe to include filter/data.h into filter/filter.h. I probably had problems when trying to incude it somewhere else, until it all finalized into filter.h
I will give more comments later.
-- 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 Thu, Jul 27, 2023 at 03:38:27PM +0200, Alexander Zubkov wrote:
Hi,
Have you had a chance to look at all this?
Hi Sorry for keeping you wait, i finally got to this patchset and merged it. I did some changes: 1) Removal of support for multiline string literals - the patch is simple but it has an awkward consequence of making incomprehensible error messages for unterminated strings. We discussed that and decided to drop it. 2) Refactor of bstrhextobin() and bstrbintohex() to be more generic and not depend bstrtobyte16(), which was removed. I never liked it anyway. Also, there is an explicit string of allowed delimiters in bstrhextobin(), for from_hex(), instead of ignoring everything that is not a letter. 3) Change val_format() to not add 'hex:' prefix when printing bytestring. It offers human readable, but not necessary back-parsable form (i.e more like Scheme function 'display' than 'write'). That is consistent with its behavior for strings, where quotation marks are also not added. 4) Use different approach for bytestring or string argument for password keys, so the target expression can know which variant was used. For other details, see the commits. Thanks for the patchset. I especially like the cf_eval() / cf_eval_int() changes. -- 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, Good news, thanks! On Thu, Aug 24, 2023 at 7:11 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Jul 27, 2023 at 03:38:27PM +0200, Alexander Zubkov wrote:
Hi,
Have you had a chance to look at all this?
Hi
Sorry for keeping you wait, i finally got to this patchset and merged it.
No problem, several days more and I would call it a birthday present. :)
I did some changes:
1) Removal of support for multiline string literals - the patch is simple but it has an awkward consequence of making incomprehensible error messages for unterminated strings. We discussed that and decided to drop it.
OK. The idea was it could be possible, to insert long hexdumps as-is or base64 outputs. So it seemed multiline strings is a good idea. But yes, missing quote could lead to some fancy parsing. I also noticed I missed "ifs->lino++; ..." there.
2) Refactor of bstrhextobin() and bstrbintohex() to be more generic and not depend bstrtobyte16(), which was removed. I never liked it anyway. Also, there is an explicit string of allowed delimiters in bstrhextobin(), for from_hex(), instead of ignoring everything that is not a letter.
OK. I also wanted to give a more freedom here to fomatting the source string, because who knows what delimeters one would like to use, not counting various types of spaces - '\t', '\n', etc. For example current allowed delimeter set does not contain '.', which is used at least for Cisco's MAC address notation: "0011.2233.4455". Also one might want use various brackets to make his blob more human-readable. But sure it can be started with a stricter variant and expanded later if there is actual demand for it.
3) Change val_format() to not add 'hex:' prefix when printing bytestring. It offers human readable, but not necessary back-parsable form (i.e more like Scheme function 'display' than 'write'). That is consistent with its behavior for strings, where quotation marks are also not added.
4) Use different approach for bytestring or string argument for password keys, so the target expression can know which variant was used.
For other details, see the commits.
Thanks for the patchset. I especially like the cf_eval() / cf_eval_int() changes.
Thanks for reviewing and merging!
-- 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."
And I forgot to ask about kw_sym. "kw_sym: FROM_HEX" definition is not needed? To provide fallback for someone using such name in config already. On Fri, Aug 25, 2023 at 3:55 AM Alexander Zubkov <green@qrator.net> wrote:
Hi,
Good news, thanks!
On Thu, Aug 24, 2023 at 7:11 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Jul 27, 2023 at 03:38:27PM +0200, Alexander Zubkov wrote:
Hi,
Have you had a chance to look at all this?
Hi
Sorry for keeping you wait, i finally got to this patchset and merged it.
No problem, several days more and I would call it a birthday present. :)
I did some changes:
1) Removal of support for multiline string literals - the patch is simple but it has an awkward consequence of making incomprehensible error messages for unterminated strings. We discussed that and decided to drop it.
OK. The idea was it could be possible, to insert long hexdumps as-is or base64 outputs. So it seemed multiline strings is a good idea. But yes, missing quote could lead to some fancy parsing. I also noticed I missed "ifs->lino++; ..." there.
2) Refactor of bstrhextobin() and bstrbintohex() to be more generic and not depend bstrtobyte16(), which was removed. I never liked it anyway. Also, there is an explicit string of allowed delimiters in bstrhextobin(), for from_hex(), instead of ignoring everything that is not a letter.
OK. I also wanted to give a more freedom here to fomatting the source string, because who knows what delimeters one would like to use, not counting various types of spaces - '\t', '\n', etc. For example current allowed delimeter set does not contain '.', which is used at least for Cisco's MAC address notation: "0011.2233.4455". Also one might want use various brackets to make his blob more human-readable. But sure it can be started with a stricter variant and expanded later if there is actual demand for it.
3) Change val_format() to not add 'hex:' prefix when printing bytestring. It offers human readable, but not necessary back-parsable form (i.e more like Scheme function 'display' than 'write'). That is consistent with its behavior for strings, where quotation marks are also not added.
4) Use different approach for bytestring or string argument for password keys, so the target expression can know which variant was used.
For other details, see the commits.
Thanks for the patchset. I especially like the cf_eval() / cf_eval_int() changes.
Thanks for reviewing and merging!
-- 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 Fri, Aug 25, 2023 at 04:20:04AM +0200, Alexander Zubkov wrote:
And I forgot to ask about kw_sym. "kw_sym: FROM_HEX" definition is not needed? To provide fallback for someone using such name in config already.
I plan to add all keywords to kw_sym (through M4 macro) in some followup commit, so no need for manual definition. -- 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! I'm not sure if it is possible to parse something like: hex(1234) both as a function call and a special bytestring depending on the symbol presense. I thought such bytestrings is needed to be defined in the lexer as a single expression for the whole sequence, because if you pass it upper to a parser as: HEX '(' <something> ')' - you'll have problem with how to parse that <something>, because it should be context-dependent. But I'm not that proficient with the parsing and I'm probably missing something. So if you have any tips on how to do it, I will be glad if you share them with me. BTW that kw_sym workaround seems like it needs to contain much more symbols. :) On Mon, Jun 12, 2023 at 2:40 PM Maria Matejka <maria.matejka@nic.cz> wrote:
Hello!
I think using hex() and base64() with adding these two tokens to the "kw_sym:" non-terminal. This way, no current config should break.
Also it may be handy for both hex() and base64() to accept any number of whitespace inside the argument, to enable configs like hex(de ad be ef) or other direct pastes from hexdumps or base64 encoders. We could then e.g. add the SSH keys for RPKI inline to the config file.
I'd prefer keeping the IPv6 and bytestrings as they are now, possibly even deprecating the current bytestring format in future. (Weak opinion on this.)
Maria On 6/12/23 14:24, Alexander Zubkov wrote:
Hello, Maria!
You suggestion for blob syntax seems good to me. I think I can try to prepare patches for that. Only one concern is that it could break some current configuration files, if they have functions with such names. Maybe it is better to use some other "brackets" to make it less possible to hit something? hex<deadbeef> or hex[deadbeef] maybe? Or even something exotic like hex|deadbeef| ?
Would you like to keep my proposed changes for IPv6 and bytestrings too? Or just introduce the new syntax?
On Mon, Jun 12, 2023 at 1:33 PM Maria Matejka <maria.matejka@nic.cz> wrote:
Hello!
This looks like a clever solution for such a problem. Thank you for the patch!
Regarding the bytestring syntax, what about adding some syntax like hex(deadbeef12345678) or even base64(...) where the user could write byte blob of any length?
Maria On 6/12/23 13:08, Alexander Zubkov via Bird-users wrote:
Hi,
Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc.
This idea is inspired by presentation by Ondřej Caletka at CSNOG, in which he noticed the lack of either PREF64 option or possibility to add custom options in various software.
Attached patch [radv-custom.patch] makes it possible to define such options with the syntax:
other type <num> <bytestring>
I can also prepare a patch for documentation if it is to be merged.
But it does solve the question with PREF64 completely. Because currently bytestring minimal length is 16 bytes, but for PREF64 we need to provide a 14-byte blob. And for minimal RA option, it have to be as short as 6 bytes.
So another patch [bytestring-short.patch] allows bytestrings to be as short as 6 bytes, when colon-notation is used: "01:02:03:04:05:06". And I kept the minimum size of 16 bytes without colons, because it can conflict with some symbol names.
The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those).
It is also not possible to define a 8-byte bytestring, because it conflicts with IPv6 address, but we are introducing short bytestrings for RA here, and 8-byte bytestrings are not strictly required for that, because possible option sizes are 8, 16, ... with payloads 2 bytes less: 6, 14, ... So if one needs a 8-byte payload, he can easily pad it with extra zeroes ":00" with the same on-the-wire result. But still, this gives one more reason for an additional syntax for bytestrings.
There also was possbility to explicitly allow only 6, 7, 9 and greater lengths of bytestrings. Or to move IPv6 regex definition before bytestring definition to make it more preferrable. I choose the second variant, but IPv6 regex is too loose now and match many things far from IPv6 notation. So I decided to provide a more strict regex definition for IPv6 addresses. Of course it is ugly, but I think it could be helpful anyway, because it does not conflict with other similar lexems, including MAC addresses.
As a downside for this, in case of some typo in IPv6 address, there will not be a meaningful error about bad IPv6 address. So I added an additional regex there, to catch lexems with hex digits and at least 2 colons to show more meaningful error for that. But I'm not sure about it.
If those changes for bytestrings are OK too, I also can prepare further patch for documentation.
Have a nice day, Alexander Zubkov
-- Maria Matejka | BIRD Team Leader | CZ.NIC, z.s.p.o.
-- Maria Matejka | BIRD Team Leader | CZ.NIC, z.s.p.o.
Alexander Zubkov via Bird-users <bird-users@network.cz> writes:
Hello, Maria!
You suggestion for blob syntax seems good to me. I think I can try to prepare patches for that. Only one concern is that it could break some current configuration files, if they have functions with such names. Maybe it is better to use some other "brackets" to make it less possible to hit something? hex<deadbeef> or hex[deadbeef] maybe? Or even something exotic like hex|deadbeef| ?
How about just using the standard 0x prefix? I.e.: 0xfc00deadbeefcafe -> 8-byte byte string fc00:dead:beef:cafe -> IPv6 address -Toke
On Mon, Jun 12, 2023 at 3:04 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
Alexander Zubkov via Bird-users <bird-users@network.cz> writes:
Hello, Maria!
You suggestion for blob syntax seems good to me. I think I can try to prepare patches for that. Only one concern is that it could break some current configuration files, if they have functions with such names. Maybe it is better to use some other "brackets" to make it less possible to hit something? hex<deadbeef> or hex[deadbeef] maybe? Or even something exotic like hex|deadbeef| ?
How about just using the standard 0x prefix?
It is already taken for integers. :)
I.e.:
0xfc00deadbeefcafe -> 8-byte byte string fc00:dead:beef:cafe -> IPv6 address
-Toke
On Mon, Jun 12, 2023 at 01:08:15PM +0200, Alexander Zubkov via Bird-users wrote:
Hi,
The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those).
Hi I already added MAC-address lexem and shortened minimum bytestring length to 9 bytes in EVPN branch (to represent 10-byte ESI) : https://gitlab.nic.cz/labs/bird/-/commit/cf0661c9762090231c9f2d973968a7ce9f9... So i would keep it at that limit and used e.g. hex:XX:YY:... syntax for shorter ones. -- 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, Jun 12, 2023 at 3:17 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 12, 2023 at 01:08:15PM +0200, Alexander Zubkov via Bird-users wrote:
Hi,
The main concern is that a 6-byte bytestring conflicts with the MAC address representation. Bird does not have the type for it currently, but who knows, it might need it in the future. So we might need some new syntax for bytestring in that case. Or it can be postponed to later time. In this case introduction of MAC-address lexems would break configs that use 6-byte bytestrings (if we want to care much about those).
Hi
I already added MAC-address lexem and shortened minimum bytestring length to 9 bytes in EVPN branch (to represent 10-byte ESI) :
https://gitlab.nic.cz/labs/bird/-/commit/cf0661c9762090231c9f2d973968a7ce9f9...
I was afraid to shorten non-colon variant, becuase somebody using the variable name "feedfacecafebeef01" might get hurt. :)
So i would keep it at that limit and used e.g. hex:XX:YY:... syntax for shorter ones.
But Maria has a good point, that it may be beneficial to be able to input the blob with spaces or even line breaks, which requires some sort of "brackets".
-- 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, Jun 12, 2023 at 01:08:15PM +0200, Alexander Zubkov via Bird-users wrote:
Hi,
Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc.
Hi Merged: https://gitlab.nic.cz/labs/bird/-/commit/9c81250c04798fd274ae9d77380e93b941a... Hopefully there is no requirement for RA options to be sorted by type, at least i did not find it in RFC 4861. The only objection from me is that 'other type' option name is kind of non-descriptive, does not indicate it is related to RA options (nor it is implicated by context). I do not really have a good idea for alternative, perhaps just 'custom option'? What do you think? Could you prepare a patch for documentation? BTW, why not to use WALK_LIST() in radv_prepare_custom()? (just noticed in now) -- 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 Fri, Jun 23, 2023, 17:47 Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Jun 12, 2023 at 01:08:15PM +0200, Alexander Zubkov via Bird-users wrote:
Hi,
Currently one can use only a predefined set of advertised options in radv protocol, that are supported by bird's configuration. It would be convenient to be able to specify other possible options at least manually as a blob so one should not wait until it is supported in the code, released, etc.
Hi
Merged:
https://gitlab.nic.cz/labs/bird/-/commit/9c81250c04798fd274ae9d77380e93b941a...
Hopefully there is no requirement for RA options to be sorted by type, at least i did not find it in RFC 4861.
The only objection from me is that 'other type' option name is kind of non-descriptive, does not indicate it is related to RA options (nor it is implicated by context). I do not really have a good idea for alternative, perhaps just 'custom option'? What do you think?
Yes, I was thinking about "custom" too. But it introduces a new keyword. So I tried too choose something suitable from available keywords. But if it is not a problem, I would prefer "custom" too as more descriptive.
Could you prepare a patch for documentation?
Sure, just will wait for the final decision about the syntax ("other" vs "custom").
BTW, why not to use WALK_LIST() in radv_prepare_custom()? (just noticed in now)
That is a good question. Actually I just used the structure of the similar function for the predefined option and didn't thought much about it. Now that you pointed that out, I remember that macro from the other parts of the code. And it seems reasonable to use it here, and probably in the "source" function too. I can prepare a patch for that.
-- 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 Sat, Jun 24, 2023 at 02:03:08AM +0200, Alexander Zubkov wrote:
On Fri, Jun 23, 2023, 17:47 Ondrej Zajicek <santiago@crfreenet.org> wrote:
The only objection from me is that 'other type' option name is kind of non-descriptive, does not indicate it is related to RA options (nor it is implicated by context). I do not really have a good idea for alternative, perhaps just 'custom option'? What do you think?
Yes, I was thinking about "custom" too. But it introduces a new keyword. So I tried too choose something suitable from available keywords. But if it is not a problem, I would prefer "custom" too as more descriptive.
I think 'custom option' would be ok. It has two arguments, thinking about it, BIRD style would be more like: custom option type 10 value 12:34:56:78:12:34:56:78:12:34:56:78:12:34:56:78; instead of just: custom option 10 12:34:56:78:12:34:56:78:12:34:56:78:12:34:56:78;
Could you prepare a patch for documentation?
Sure, just will wait for the final decision about the syntax ("other" vs "custom").
okay.
BTW, why not to use WALK_LIST() in radv_prepare_custom()? (just noticed in now)
That is a good question. Actually I just used the structure of the similar function for the predefined option and didn't thought much about it. Now that you pointed that out, I remember that macro from the other parts of the code. And it seems reasonable to use it here, and probably in the "source" function too. I can prepare a patch for that.
Okay. I see the 'source' funcions has more complex structure (two nested whiles), so perhaps it does not fit to them. -- 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."
Attached the patch with the new syntax for custom options and to use WALK_LIST. On Sat, Jun 24, 2023 at 3:32 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Sat, Jun 24, 2023 at 02:03:08AM +0200, Alexander Zubkov wrote:
On Fri, Jun 23, 2023, 17:47 Ondrej Zajicek <santiago@crfreenet.org> wrote:
The only objection from me is that 'other type' option name is kind of non-descriptive, does not indicate it is related to RA options (nor it is implicated by context). I do not really have a good idea for alternative, perhaps just 'custom option'? What do you think?
Yes, I was thinking about "custom" too. But it introduces a new keyword. So I tried too choose something suitable from available keywords. But if it is not a problem, I would prefer "custom" too as more descriptive.
I think 'custom option' would be ok. It has two arguments, thinking about it, BIRD style would be more like:
custom option type 10 value 12:34:56:78:12:34:56:78:12:34:56:78:12:34:56:78;
instead of just:
custom option 10 12:34:56:78:12:34:56:78:12:34:56:78:12:34:56:78;
Could you prepare a patch for documentation?
Sure, just will wait for the final decision about the syntax ("other" vs "custom").
okay.
BTW, why not to use WALK_LIST() in radv_prepare_custom()? (just noticed in now)
That is a good question. Actually I just used the structure of the similar function for the predefined option and didn't thought much about it. Now that you pointed that out, I remember that macro from the other parts of the code. And it seems reasonable to use it here, and probably in the "source" function too. I can prepare a patch for that.
Okay. I see the 'source' funcions has more complex structure (two nested whiles), so perhaps it does not fit to them.
-- 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, Jun 26, 2023 at 03:01:55AM +0200, Alexander Zubkov wrote:
Attached the patch with the new syntax for custom options and to use WALK_LIST.
Thanks, merged: https://gitlab.nic.cz/labs/bird/-/commit/ccfa48a24aea64d1a844fb97cfe15965197... -- 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 (4)
-
Alexander Zubkov -
Maria Matejka -
Ondrej Zajicek -
Toke Høiland-Jørgensen