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