Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 wrote: > > Hi, > > Good news, thanks! > > On Thu, Aug 24, 2023 at 7:11 PM Ondrej Zajicek 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: santi...@crfreenet.org) > > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) > > "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
Hi, Good news, thanks! On Thu, Aug 24, 2023 at 7:11 PM Ondrej Zajicek 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: santi...@crfreenet.org) > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) > "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
Hi, Have you had a chance to look at all this? On Fri, Jul 7, 2023 at 12:55 AM Alexander Zubkov 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 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 wrote: > > > > > > On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov wrote: > > > > > > > > On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov > > > > wrote: > > > > > > > > > > On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek > > > > > 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 > > > > > > > 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 be
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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 wrote: > > > > On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov wrote: > > > > > > On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov wrote: > > > > > > > > On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek > > > > 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 > > > > > > 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(..)
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 wrote:
>
> On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov wrote:
> >
> > On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov wrote:
> > >
> > > On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek
> > > 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
> > > > > 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'
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Tue, Jun 27, 2023 at 2:13 AM Alexander Zubkov wrote:
>
> On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov wrote:
> >
> > On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek
> > 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
> > > > 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: santi...@crfreenet.org)
> > > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> > > "To err is human -- to blame it on a computer is even more so."
From 63f9dcc924599915a93cb71f1f9e9cbb8ebd087b Mon Sep 17 00:00:00 2001
From: Alexander Zubkov
Date: Thu, 29 Jun 2023 13:00:12 +0200
Subject: [PATCH] Conf: use more generic approach for intra-config expression
evaluation
Replace f_eval_int() function with a type-generic variant: cf_eval().
And implement similar fuction: cf_eval_int() via inline call to cf_eval().
---
conf/confbase.Y | 4 ++--
filter/config.Y | 8
filter/filter.c | 22 +++---
filter/filter.h | 8 +---
nest/config.Y | 2 +-
5 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/conf/confbase.Y b/conf/confbase.Y
index da750d38..e109ddf5 100644
--- a/conf/confbase.Y
+++ b/conf/confbase.Y
@@ -155,14 +155,14 @@ conf: definition ;
definition:
DEFINE symbol '=' term ';' {
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Mon, Jun 26, 2023 at 5:54 PM Alexander Zubkov wrote:
>
> On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek 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
> > > 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: santi...@crfreenet.org)
> > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> > "To err is human -- to blame it on a computer is even more so."
From 41542bbffebe351b1df96210849a1a22b3c74474 Mon Sep 17 00:00:00 2001
From: Alexander Zubkov
Date: Tue, 27 Jun 2023 00:53:05 +0200
Subject: [PATCH] Use more proper pointers to constant bytestrings
---
conf/confbase.Y | 2 +-
proto/radv/config.Y | 2 +-
proto/radv/radv.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/conf/confbase.Y b/conf/confbase.Y
index 3e8f5807..3dd5fed7 100644
--- a/conf/confbase.Y
+++ b/conf/confbase.Y
@@ -94,7 +94,7 @@ CF_DECLS
struct channel_limit cl;
struct timeformat *tf;
mpls_label_stack *mls;
- struct bytestring *bs;
+ const struct bytestring *bs;
}
%token END CLI_MARKER INVALID_TOKEN ELSECOL DDOT
diff --git a/proto/radv/config.Y b/proto/radv/config.Y
index db683194..eeafe6f4 100644
--- a/proto/radv/config.Y
+++ b/proto/radv/config.Y
@@ -26,7 +26,7 @@ static list radv_dns_list; /* Used by radv_rdnss and radv_dnssl */
static u8 radv_mult_val; /* Used by radv_mult for second return value */
static inline void
-radv_add_to_custom_list(list *l, int type, struct bytestring *payload)
+radv_add_to_custom_list(list *l, int type, const struct bytestring *payload)
{
if (type < 0 || type > 255) cf_error("RA cusom type must be in range 0-255");
struct radv_custom_config *cf = cfg_allocz(sizeof(struct radv_custom_config));
diff --git a/proto/radv/radv.h b/proto/radv/radv.h
index 8c716158..2baf0bad 100644
--- a/proto/radv/radv.h
+++ b/proto/radv/radv.h
@@ -129,7 +129,7 @@ struct radv_custom_config
{
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Mon, Jun 26, 2023 at 5:43 PM Ondrej Zajicek 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
> > 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Mon, Jun 26, 2023 at 03:24:47AM +0200, Alexander Zubkov wrote:
> On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek 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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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/ccfa48a24aea64d1a844fb97cfe15965197c0908 -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Sat, Jun 24, 2023 at 3:16 PM Ondrej Zajicek 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
commit 7cc6700c64e94019df1743de685c84dbca85b406
Author: Alexander Zubkov
Date: Mon Jun 26 02:38:54 2023 +0200
Conf: make bytestring a type
diff --git a/bird-gdb.py b/bird-gdb.py
index 3cf65a9c..262035dc 100644
--- a/bird-gdb.py
+++ b/bird-gdb.py
@@ -34,6 +34,7 @@ class BIRDFValPrinter(BIRDPrinter):
"T_IP": "ip",
"T_NET": "net",
"T_STRING": "s",
+"T_BYTESTRING": "bs",
"T_PATH_MASK": "path_mask",
"T_PATH": "ad",
"T_CLIST": "ad",
diff --git a/conf/cf-lex.l b/conf/cf-lex.l
index 9025a84d..59b88bd5 100644
--- a/conf/cf-lex.l
+++ b/conf/cf-lex.l
@@ -256,37 +256,26 @@ WHITE [ \t]
}
({XIGIT}{2}){16,}|{XIGIT}{2}(:{XIGIT}{2}){15,}|hex:({XIGIT}{2}(:?{XIGIT}{2})*)? {
- char *s, *sb = yytext;
- size_t len = 0, i;
+ char *sb = yytext;
+ size_t len = 0;
struct bytestring *bytes;
- byte *b;
/* skip 'hex:' prefix */
if (sb[0] == 'h' && sb[1] == 'e' && sb[2] == 'x' && sb[3] == ':')
sb += 4;
- s = sb;
- while (*s) {
-len++;
-s += 2;
-if (*s == ':')
- s++;
- }
+ errno = 0;
+ len = bstrhextobin(sb, 0);
+ if (errno || len == (size_t)-1)
+cf_error("Invalid hex string");
+
bytes = cfg_allocz(sizeof(*bytes) + len);
bytes->length = len;
- b = &bytes->data[0];
- s = sb;
- errno = 0;
- for (i = 0; i < len; i++) {
-*b = bstrtobyte16(s);
-if (errno == ERANGE)
- cf_error("Invalid hex string");
-b++;
-s += 2;
-if (*s == ':')
- s++;
- }
+ bstrhextobin(sb, bytes->data);
+ if (errno)
+cf_error("Invalid hex string");
+
cf_lval.bs = bytes;
return BYTESTRING;
}
@@ -361,7 +350,6 @@ else: {
quoted_buffer_init();
}
-\n cf_error("Unterminated string");
<> cf_error("Unterminated string");
["] {
BEGIN(INITIAL);
@@ -370,7 +358,7 @@ else: {
return TEXT;
}
-. BUFFER_PUSH(quoted_buffer) = yytext[0];
+(.|\n) BUFFER_PUSH(quoted_buffer) = yytext[0];
<> { if (check_eof()) return END; }
diff --git a/conf/confbase.Y b/conf/confbase.Y
index 3e8f5807..ebaa93b8 100644
--- a/conf/confbase.Y
+++ b/conf/confbase.Y
@@ -94,7 +94,7 @@ CF_DECLS
struct channel_limit cl;
struct timeformat *tf;
mpls_label_stack *mls;
- struct bytestring *bs;
+ const struct bytestring *bs;
}
%token END CLI_MARKER INVALID_TOKEN ELSECOL DDOT
@@ -117,9 +117,12 @@ CF_DECLS
%type label_stack_start label_stack
%type text opttext
+%type bytestring
%type symbol
%type kw_sym
+
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
Hello! On Sat, Jun 24, 2023 at 3:30 PM Maria Matejka 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.
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 wrote:
>
> On Sat, Jun 24, 2023 at 02:03:08AM +0200, Alexander Zubkov wrote:
> > On Fri, Jun 23, 2023, 17:47 Ondrej Zajicek 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
diff --git a/proto/radv/config.Y b/proto/radv/config.Y
index 5c213d50..81dc415a 100644
--- a/proto/radv/config.Y
+++ b/proto/radv/config.Y
@@ -42,7 +42,7 @@ CF_KEYWORDS(RADV, PREFIX, INTERFACE, MIN, MAX, RA, DELAY, INTERVAL, SOLICITED,
RETRANS, TIMER, CURRENT, HOP, LIMIT, DEFAULT, VALID, PREFERRED, MULT,
LIFETIME, SKIP, ONLINK, AUTONOMOUS, RDNSS, DNSSL, NS, DOMAIN, LOCAL,
TRIGGER, SENSITIVE, PREFERENCE, LOW, MEDIUM, HIGH, PROPAGATE, ROUTE,
- ROUTES, RA_PREFERENCE, RA_LIFETIME)
+ ROUTES, RA_PREFERENCE, RA_LIFETIME, CUSTOM, OPTION, VALUE)
CF_ENUM(T_ENUM_RA_PREFERENCE, RA_PREF_, LOW, MEDIUM, HIGH)
@@ -50,6 +50,8 @@ CF_ENUM(T_ENUM_RA_PREFERENCE, RA_PREF_, LOW, MEDIUM, HIGH)
CF_GRAMMAR
+kw_sym: CUSTOM | OPTION | VALUE ;
+
proto: radv_proto ;
radv_proto_start: proto_start RADV
@@ -71,7 +73,7 @@ radv_proto_item:
| PREFIX radv_prefix { add_tail(&RADV_CFG->pref_list, NODE this_radv_prefix); }
| RDNSS { init_list(&radv_dns_list); } radv_rdnss { add_tail_list(&RADV_CFG->rdnss_list, &radv_dns_list); }
| DNSSL { init_list(&radv_dns_list); } radv_dnssl { add_tail_list(&RADV_CFG->dnssl_list, &radv_dns_list); }
- | OTHER TYPE expr BYTESTRING { radv_add_to_custom_list(&RADV_CFG->custom_list, $3, $4); }
+ | CUSTOM OPTION TYPE expr VALUE BYTESTRING { radv_add_to_custom_list(&RADV_CFG->custom_list, $4, $6); }
| TRIGGER net_ip6 { RADV_CFG->trigger = $2; }
| PROPAGATE ROUTES bool { RADV_CFG->propagate_routes = $3; }
;
@@ -136,10 +138,10 @@ radv_iface_item:
| PREFIX radv_prefix { add_tail(&RADV_IFACE->pref_list, NODE this_radv_prefix); }
| RDNSS { init_list(&radv_dns_list); } radv_rdnss { add_tail_list(&RADV_IFACE->rdnss_list, &radv_dns_list); }
| DNSSL { init_list(&radv_dns_list); } radv_dnssl { add_tail_list(&RADV_IFACE->dnssl_list, &radv_dns_list); }
- | OTHER TYPE expr BYTESTRING { radv_add_to_custom_list(&RADV_IFACE->custom_list, $3, $4); }
+ | CUSTOM OPTION TYPE expr VALUE BYTESTRING { radv_add_to_custom_list(&RADV_IFACE->custom_list, $4, $6); }
| RDNSS LOCAL bool { RADV_IFACE->rdnss_local = $3; }
| DNSSL LOCAL bool { RADV_IFACE->dnssl_local = $3; }
- | OTHER LOCAL bool { RADV_IFACE->custom_local = $3; }
+ | CUSTOM OPTION LOCAL bool { RADV_IFACE->custom_local = $4; }
;
radv_preference:
diff --git a/proto/radv/packets.c b/proto/radv/packets.c
index d1f86ec1..77c98794 100644
--- a/proto/radv/packets.c
+++ b/proto/radv/packets.c
@@ -264,9 +264,8 @@ radv_prepare_dnssl(struct radv_iface *ifa, list *dnssl_list, char **buf, char *b
static int
radv_prepare_custom(struct radv_iface *ifa, list *custom_list, char **buf, char *bufend)
{
- struct radv_custom_config *ccf = HEAD(*custom_list);
-
- while(NODE_VALID(ccf))
+ struct radv_custom_config *ccf;
+ WALK_LIST(ccf, *custom_list)
{
struct radv_opt_custom *op = (void *) *buf;
/* Add 2 octets for type and size and 8 - 1 for ceiling the division up to 8 octets */
@@ -280,7 +
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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.
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Sat, Jun 24, 2023 at 02:03:08AM +0200, Alexander Zubkov wrote:
> On Fri, Jun 23, 2023, 17:47 Ondrej Zajicek 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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Void values in variables (Was: Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex)
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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Fri, Jun 23, 2023, 18:30 Ondrej Zajicek 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/65d6a525944faa3f77041419991d77106d8f0a0d
>
> 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
>
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Fri, Jun 23, 2023, 17:47 Ondrej Zajicek 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/9c81250c04798fd274ae9d77380e93b941ac2d7f
>
> 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
>
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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/65d6a525944faa3f77041419991d77106d8f0a0d
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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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/9c81250c04798fd274ae9d77380e93b941ac2d7f 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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
Hi,
Please give some feedback.
On Thu, Jun 15, 2023 at 3:57 AM 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 {}
> 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 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
> > wrote:
> > >
> > > On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
> > > > On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek
> > > > wrote:
> > > >
> > > > > We agreed on keeping existing format for suffiently long hex
> > > > > bytestrings,
> > > > > using
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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 wrote:
> >
> > On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
> > > On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek 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 prio
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 wrote:
>
> On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
> > On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
commit a0d088b49510db8f968a99b7e1a5f7f1242e199d
Author: Alexander Zubkov
Date: Tue Jun 13 22:19:28 2023 +0200
Conf: allow to provide a bytestring of any length with 'hex:' prefix
diff --git a/conf/cf-lex.l b/conf/cf-lex.l
index 9555949d..9025a84d 100644
--- a/conf/cf-lex.l
+++ b/conf/cf-lex.l
@@ -255,12 +255,17 @@ WHITE [ \t]
return IP4;
}
-{XIGIT}{2}((:{XIGIT}{2}){15,}|({XIGIT}{2}){15,}) {
- char *s = yytext;
+({XIGIT}{2}){16,}|{XIGIT}{2}(:{XIGIT}{2}){15,}|hex:({XIGIT}{2}(:?{XIGIT}{2})*)? {
+ char *s, *sb = yytext;
size_t len = 0, i;
struct bytestring *bytes;
byte *b;
+ /* skip 'hex:' prefix */
+ if (sb[0] == 'h' && sb[1] == 'e' && sb[2] == 'x' && sb[3] == ':')
+sb += 4;
+
+ s = sb;
while (*s) {
len++;
s += 2;
@@ -271,7 +276,7 @@ WHITE [ \t]
bytes->length = len;
b = &bytes->data[0];
- s = yytext;
+ s = sb;
errno = 0;
for (i = 0; i < len; i++) {
*b = bstrtobyte16(s);
commit eead1fd2f7193bed2bbece8aeebfa04571030f6e
Author: Alexander Zubkov
Date: Wed Jun 14 00:08:23 2023 +0200
Conf: new bytestring syntax hex("..."), allow multiline strings
diff --git a/conf/cf-lex.l b/conf/cf-lex.l
index 9025a84d..dd607380 100644
--- a/conf/cf-lex.l
+++ b/conf/cf-lex.l
@@ -256,37 +256,26 @@ WHITE [ \t]
}
({XIGIT}{2}){16,}|{XIGIT}{2}(:{XIGIT}{2}){15,}|hex:({XIGIT}{2}(:?{XIGIT}{2})*)? {
- char *s, *sb = yytext;
- size_t len = 0, i;
+ char *str = yytext;
+ size_t len;
struct bytestring *bytes;
- byte *b;
/* skip 'hex:' prefix */
- if (sb[0] == 'h' && sb[1] == 'e' && sb[2] == 'x' && sb[3] == ':')
-sb += 4;
-
- s = sb;
- while (*s) {
-len++;
-s += 2;
-if (*s == ':')
- s++;
- }
+ if (str[0] == 'h' && str[1] == 'e' && str[2] == 'x' && str[3] == ':')
+str += 4;
+
+ errno = 0;
+ len = bstrhextobin(str, 0);
+ if (errno || len == (size_t)-1)
+cf_error("Invalid hex string");
+
bytes = cfg_allocz(sizeof(*bytes) + len);
bytes->length = len;
- b = &bytes->data[0];
- s = sb;
- errno = 0;
- for (i = 0; i < len; i++) {
-*b = bstrtobyte16(s);
-if (errno == ERANGE)
- cf_error("Invalid hex string");
-b++;
-s += 2;
-if (*s == ':')
- s++;
- }
+ bstrhextobin(str, bytes->data);
+ if (errno)
+cf_error("Invalid hex string");
+
cf_lval.bs = bytes;
return BYTESTRING;
}
@@ -361,7 +350,6 @@ else: {
quoted_buffer_init();
}
-\n cf_error("Unterminated string");
<> cf_error("Unterminated string");
["] {
BEGIN(INITIAL);
@@ -370,7 +358,7 @@ else: {
return TEXT;
}
-. BUFFER_PUSH(quoted_buffer) = yytext[0];
+(.|\n) BUFFER_PUSH(quoted_buffer) = yyt
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Tue, Jun 13, 2023 at 05:34:18PM +0200, Alexander Zubkov wrote:
> On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek 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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Tue, Jun 13, 2023, 16:07 Ondrej Zajicek 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
>
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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 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: santi...@crfreenet.org)
>> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
>> "To err is human -- to blame it on a computer is even more so."
--
Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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: santi...@crfreenet.org)
> OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Mon, Jun 12, 2023 at 3:17 PM Ondrej Zajicek 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/cf0661c9762090231c9f2d973968a7ce9f98407e > > 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: santi...@crfreenet.org) > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) > "To err is human -- to blame it on a computer is even more so." >
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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: santi...@crfreenet.org) >OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) >"To err is human -- to blame it on a computer is even more so." -- Maria Matejka (she/her) | BIRD Team Leader | CZ.NIC, z.s.p.o.
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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/cf0661c9762090231c9f2d973968a7ce9f98407e 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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
On Mon, Jun 12, 2023 at 3:04 PM Toke Høiland-Jørgensen wrote: > Alexander Zubkov via Bird-users 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 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 >
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 '(' ')' - you'll have problem with how to
parse that , 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 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 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
> 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
>>
>> 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
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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: santi...@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
Alexander Zubkov via Bird-users 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 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
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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 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 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.
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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 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 > > 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. > >
Re: [PATCH] adding custom options in radv protocol, strict ipv6 regex
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 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.
[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
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
commit 2b7908712dda313e0a97740751315e99ec0d06c0
Author: Alexander Zubkov
Date: Mon Jun 12 11:52:13 2023 +0200
RAdv: allow adding custom options with payload as hexadecimal string
Allows to send RA options that do not have specific configure commands
defined for it by specifying its plain payload manually.
diff --git a/proto/radv/config.Y b/proto/radv/config.Y
index 8d4a3ab9..5c213d50 100644
--- a/proto/radv/config.Y
+++ b/proto/radv/config.Y
@@ -25,6 +25,15 @@ static struct radv_dnssl_config this_radv_dnssl;
static list radv_dns_list; /* Used by radv_rdnss and radv_dnssl */
static u8 radv_mult_val; /* Used by radv_mult for second return value */
+static inline void
+radv_add_to_custom_list(list *l, int type, struct bytestring *payload)
+{
+ if (type < 0 || type > 255) cf_error("RA cusom type must be in range 0-255");
+ struct radv_custom_config *cf = cfg_allocz(sizeof(struct radv_custom_config));
+ add_tail(l, NODE cf);
+ cf->type = type;
+ cf->payload = payload;
+}
CF_DECLS
@@ -52,6 +61,7 @@ radv_proto_start: proto_start RADV
init_list(&RADV_CFG->pref_list);
init_list(&RADV_CFG->rdnss_list);
init_list(&RADV_CFG->dnssl_list);
+ init_list(&RADV_CFG->custom_list);
};
radv_proto_item:
@@ -61,6 +71,7 @@ radv_proto_item:
| PREFIX radv_prefix { add_tail(&RADV_CFG->pref_list, NODE this_radv_prefix); }
| RDNSS { init_list(&radv_dns_list); } radv_rdnss { add_tail_list(&RADV_CFG->rdnss_list, &radv_dns_list); }
| DNSSL { init_list(&radv_dns_list); } radv_dnssl { add_tail_list(&RADV_CFG->dnssl_list, &radv_dns_list); }
+ | OTHER TYPE expr BYTESTRING { radv_add_to_custom_list(&RADV_CFG->custom_list, $3, $4); }
| TRIGGER net_ip6 { RADV_CFG->trigger = $2; }
| PROPAGATE ROUTES bool { RADV_CFG->propagate_routes = $3; }
;
@@ -82,6 +93,7 @@ radv_iface_start:
init_list(&RADV_IFACE->pref_list);
init_list(&RADV_IFACE->rdnss_list);
init_list(&RADV_IFACE->dnssl_list);
+ init_list(&RADV_IFACE->custom_list);
RADV_IFACE->min_ra_int = (u32) -1; /* undefined
