Thanks Ales, This address set is not present in SB DB. -Srini
On Sun, May 12, 2024 at 10:26 PM Ales Musil <[email protected]> wrote: > > > On Sun, May 12, 2024 at 11:17 PM Sri kor <[email protected]> wrote: > >> Hi @[email protected] <[email protected]> @Ales Musil <[email protected]> , >> Currently we are on OVN 23.09. We are facing the following syntax >> error with the address_set.From the diff, looks like you address this fix. >> Is this something expected? >> >> *2024-05-11T17:35:28.694Z|153935|lflow|WARN|error parsing match "reg0[7] >> == 1 && ((ip4.src == >> $a_781ac228_38ae_4f04_9cc1_707543df50c8_addr_set_b8f7bf7f_83b3_48bc_a0d5_bc5d55438e4a) >> && (172.27.0.0 <= ip4.dst <= 172.27.255.255) && (1 <= tcp.src <= 65535 || 1 >> <= udp.src <= 65535) && (tcp.dst == 22 || udp.dst == 22) && outport == >> @a_781ac228_38ae_4f04_9cc1_707543df50c8_pg_0ea55446_0df6_4358_8442_56893854004c)": >> Syntax error at >> `$a_781ac228_38ae_4f04_9cc1_707543df50c8_addr_set_b8f7bf7f_83b3_48bc_a0d5_bc5d55438e4a' >> expecting address set name.* >> >> >> *Here are the transaction details .* >> >> *[{Op:insert Table:ACL Row:map[action:allow-related direction:to-lport >> external_ids:{GoMap:map[crusoe_fw_rule_id:770b6929-88d1-4659-ba3b-ed84ccfd71a0 >> crusoe_vpc_id:0ea55446-0df6-4358-8442-56893854004c >> customer_id:781ac228-38ae-4f04-9cc1-707543df50c8]} log:false match:(ip4.src >> == >> $a_781ac228_38ae_4f04_9cc1_707543df50c8_addr_set_b8f7bf7f_83b3_48bc_a0d5_bc5d55438e4a) >> && (172.27.0.0 <= ip4.dst <= 172.27.255.255) && (1 <= tcp.src <= 65535 || 1 >> <= udp.src <= 65535) && (tcp.dst == 22 || udp.dst == 22) && outport == >> @a_781ac228_38ae_4f04_9cc1_707543df50c8_pg_0ea55446_0df6_4358_8442_56893854004c >> name:{GoSet:[a_781ac228_38ae_4f04_9cc1_707543df50c8_acl_770b6929_88d1_4659_b]} >> priority:2000] Rows:[] Columns:[] Mutations:[] Timeout:<nil> Where:[] >> Until: Durable:<nil> Comment:<nil> Lock:<nil> >> UUIDName:_641d4ca1_7596_4004_b577_0f34e0bab87d} {Op:mutate Table:Port_Group >> Row:map[] Rows:[] Columns:[] Mutations:[{Column:acls Mutator:insert >> Value:{GoSet:[{GoUUID:_641d4ca1_7596_4004_b577_0f34e0bab87d}]}}] >> Timeout:<nil> Where:[where column _uuid == >> {00383922-81ce-4491-9115-6ea2dfc79aea}] Until: Durable:<nil> Comment:<nil> >> Lock:<nil> UUIDName:} {Op:mutate Table:NB_Global Row:map[] Rows:[] >> Columns:[] Mutations:[{Column:nb_cfg Mutator:+= Value:1}] Timeout:<nil> >> Where:[where column _uuid == {be2921c1-99ae-423b-a69c-a43e67d8424e}] Until: >> Durable:<nil> Comment:<nil> Lock:<nil> UUIDName:} {Op:select >> Table:NB_Global Row:map[] Rows:[] Columns:[nb_cfg] Mutations:[] >> Timeout:<nil> Where:[] Until: Durable:<nil> Comment:<nil> Lock:<nil> >> UUIDName:}]* >> >> >> *Thanks* >> >> *Srini* >> >> > Hi, > > this seems to be unrelated to the patch. It looks like the controller is > not able to recognize the address set name and doesn't parse it correctly. > Is this address set present in the SB DB? > > Thanks, > Ales > > On Fri, May 10, 2024 at 9:43 AM Numan Siddique <[email protected]> wrote: >> >>> On Fri, May 10, 2024 at 12:39 PM Han Zhou <[email protected]> wrote: >>> > >>> > On Fri, May 10, 2024 at 9:23 AM Numan Siddique <[email protected]> wrote: >>> > > >>> > > On Fri, May 10, 2024 at 2:37 AM Han Zhou <[email protected]> wrote: >>> > > > >>> > > > On Thu, May 9, 2024 at 10:32 AM Mark Michelson < >>> [email protected]> >>> > wrote: >>> > > > > >>> > > > > On 5/7/24 02:12, Han Zhou wrote: >>> > > > > > >>> > > > > > >>> > > > > > On Mon, May 6, 2024 at 10:37 PM Ales Musil <[email protected] >>> > > > > > <mailto:[email protected]>> wrote: >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > > > > > > On Mon, May 6, 2024 at 8:41 PM Han Zhou <[email protected] >>> > > > > > <mailto:[email protected]>> wrote: >>> > > > > > >> >>> > > > > > >> >>> > > > > > >> >>> > > > > > >> On Thu, May 2, 2024 at 10:35 PM Ales Musil < >>> [email protected] >>> > > > > > <mailto:[email protected]>> wrote: >>> > > > > > >> > >>> > > > > > >> > On Thu, May 2, 2024 at 6:23 PM Han Zhou <[email protected] >>> > > > > > <mailto:[email protected]>> wrote: >>> > > > > > >> > > >>> > > > > > >> > > >>> > > > > > >> > > >>> > > > > > >> > > On Thu, May 2, 2024 at 6:29 AM Ales Musil < >>> [email protected] >>> > > > > > <mailto:[email protected]>> wrote: >>> > > > > > >> > > > >>> > > > > > >> > > > Instead of tracking address set per struct >>> > expr_constant_set >>> > > > > > track it >>> > > > > > >> > > > per individual struct expr_constant. This allows >>> more fine >>> > > > grained >>> > > > > > >> > > > control for I-P processing of address sets in >>> controller. >>> > It >>> > > > > > helps with >>> > > > > > >> > > > scenarios like matching on two address sets in one >>> > expression >>> > > > e.g. >>> > > > > > >> > > > "ip4.src == {$as1, $as2}". This allows any addition >>> or >>> > > > removal of >>> > > > > > >> > > > individual adress from the set to be incrementally >>> > processed >>> > > > > > instead >>> > > > > > >> > > > of reprocessing all the flows. >>> > > > > > >> > > > >>> > > > > > >> > > > This unfortunately doesn't help with the following >>> flows: >>> > > > > > >> > > > "ip4.src == $as1 && ip4.dst == $as2" >>> > > > > > >> > > > "ip4.src == $as1 || ip4.dst == $as2" >>> > > > > > >> > > > >>> > > > > > >> > > > The memory impact should be minimal as there is only >>> > increase >>> > > > > > of 8 bytes >>> > > > > > >> > > > per the struct expr_constant. >>> > > > > > >> > > > >>> > > > > > >> > > > Reported-at: >>> https://issues.redhat.com/browse/FDP-509 >>> > > > > > <https://issues.redhat.com/browse/FDP-509> >>> > > > > > >> > > > Signed-off-by: Ales Musil <[email protected] >>> > > > > > <mailto:[email protected]>> >>> > > > > > >> > > > --- >>> > > > > > >> > > > v4: Rebase on top of current main. >>> > > > > > >> > > > Update the "lflow_handle_addr_set_update" comment >>> > > > > > according to suggestion from Han. >>> > > > > > >> > > >>> > > > > > >> > > Thanks Ales. I updated the commit message for the >>> same, and >>> > > > > > applied to main branch. >>> > > > > > >> > > >>> > > > > > >> > > Regards, >>> > > > > > >> > > Han >>> > > > > > >> > > >>> > > > > > >> > > > v3: Rebase on top of current main. >>> > > > > > >> > > > Address comments from Han: >>> > > > > > >> > > > - Adjust the comment for >>> > "lflow_handle_addr_set_update" to >>> > > > > > include remaning corner cases. >>> > > > > > >> > > > - Make sure that the flows are consistent >>> between I-P >>> > and >>> > > > > > recompute. >>> > > > > > >> > > > v2: Rebase on top of current main. >>> > > > > > >> > > > Adjust the comment for I-P optimization. >>> > > > > > >> > > > --- >>> > > > > > >> > > > controller/lflow.c | 11 ++--- >>> > > > > > >> > > > include/ovn/actions.h | 2 +- >>> > > > > > >> > > > include/ovn/expr.h | 46 ++++++++++--------- >>> > > > > > >> > > > lib/actions.c | 20 ++++----- >>> > > > > > >> > > > lib/expr.c | 99 >>> > > > > > +++++++++++++++++------------------------ >>> > > > > > >> > > > tests/ovn-controller.at <http://ovn-controller.at> >>> | 79 >>> > > > > > +++++++++++++++++++++++++++++--- >>> > > > > > >> > > > 6 files changed, 154 insertions(+), 103 deletions(-) >>> > > > > > >> > > > >>> > > > > > >> > > > diff --git a/controller/lflow.c b/controller/lflow.c >>> > > > > > >> > > > index 760ec0b41..1e05665a1 100644 >>> > > > > > >> > > > --- a/controller/lflow.c >>> > > > > > >> > > > +++ b/controller/lflow.c >>> > > > > > >> > > > @@ -278,7 +278,7 @@ lflow_handle_changed_flows(struct >>> > > > > > lflow_ctx_in *l_ctx_in, >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > static bool >>> > > > > > >> > > > -as_info_from_expr_const(const char *as_name, const >>> union >>> > > > > > expr_constant *c, >>> > > > > > >> > > > +as_info_from_expr_const(const char *as_name, const >>> struct >>> > > > > > expr_constant *c, >>> > > > > > >> > > > struct addrset_info >>> *as_info) >>> > > > > > >> > > > { >>> > > > > > >> > > > as_info->name = as_name; >>> > > > > > >> > > > @@ -644,14 +644,11 @@ as_update_can_be_handled(const >>> char >>> > > > > > *as_name, struct addr_set_diff *as_diff, >>> > > > > > >> > > > * generated. >>> > > > > > >> > > > * >>> > > > > > >> > > > * - The sub expression of the address set is >>> > combined >>> > > > > > with other sub- >>> > > > > > >> > > > - * expressions/constants, usually because of >>> > > > > > disjunctions between >>> > > > > > >> > > > - * sub-expressions/constants, e.g.: >>> > > > > > >> > > > + * expressions/constants on different fields, >>> > e.g.: >>> > > > > > >> > > > * >>> > > > > > >> > > > * ip.src == $as1 || ip.dst == $as2 >>> > > > > > >> > > > - * ip.src == {$as1, $as2} >>> > > > > > >> > > > - * ip.src == {$as1, ip1} >>> > > > > > >> > > > * >>> > > > > > >> > > > - * All these could have been split into >>> separate >>> > > > lflows. >>> > > > > > >> > > > + * This could have been split into separate >>> > lflows. >>> > > > > > >> > > > * >>> > > > > > >> > > > * - Conjunctions overlapping between lflows, >>> which >>> > can >>> > > > > > be caused by >>> > > > > > >> > > > * overlapping address sets or same address >>> set >>> > used >>> > > > > > by multiple lflows >>> > > > > > >> > > > @@ -714,7 +711,7 @@ >>> lflow_handle_addr_set_update(const >>> > char >>> > > > > > *as_name, >>> > > > > > >> > > > if (as_diff->deleted) { >>> > > > > > >> > > > struct addrset_info as_info; >>> > > > > > >> > > > for (size_t i = 0; i < >>> > > > > > as_diff->deleted->n_values; i++) { >>> > > > > > >> > > > - union expr_constant *c = >>> > > > > > &as_diff->deleted->values[i]; >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > > > &as_diff->deleted->values[i]; >>> > > > > > >> > > > if >>> (!as_info_from_expr_const(as_name, c, >>> > > > > > &as_info)) { >>> > > > > > >> > > > continue; >>> > > > > > >> > > > } >>> > > > > > >> > > > diff --git a/include/ovn/actions.h >>> b/include/ovn/actions.h >>> > > > > > >> > > > index ae0864fdd..88cf4de79 100644 >>> > > > > > >> > > > --- a/include/ovn/actions.h >>> > > > > > >> > > > +++ b/include/ovn/actions.h >>> > > > > > >> > > > @@ -241,7 +241,7 @@ struct ovnact_next { >>> > > > > > >> > > > struct ovnact_load { >>> > > > > > >> > > > struct ovnact ovnact; >>> > > > > > >> > > > struct expr_field dst; >>> > > > > > >> > > > - union expr_constant imm; >>> > > > > > >> > > > + struct expr_constant imm; >>> > > > > > >> > > > }; >>> > > > > > >> > > > >>> > > > > > >> > > > /* OVNACT_MOVE, OVNACT_EXCHANGE. */ >>> > > > > > >> > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h >>> > > > > > >> > > > index c48f82398..e54edb5bf 100644 >>> > > > > > >> > > > --- a/include/ovn/expr.h >>> > > > > > >> > > > +++ b/include/ovn/expr.h >>> > > > > > >> > > > @@ -368,7 +368,7 @@ bool expr_relop_from_token(enum >>> > lex_type >>> > > > > > type, enum expr_relop *relop); >>> > > > > > >> > > > struct expr { >>> > > > > > >> > > > struct ovs_list node; /* In parent >>> EXPR_T_AND >>> > or >>> > > > > > EXPR_T_OR if any. */ >>> > > > > > >> > > > enum expr_type type; /* Expression type. >>> */ >>> > > > > > >> > > > - char *as_name; /* Address set name. >>> > Null if >>> > > > > > it is not an >>> > > > > > >> > > > + const char *as_name; /* Address set name. >>> > Null if >>> > > > > > it is not an >>> > > > > > >> > > > address set. */ >>> > > > > > >> > > > >>> > > > > > >> > > > union { >>> > > > > > >> > > > @@ -505,40 +505,42 @@ enum expr_constant_type { >>> > > > > > >> > > > }; >>> > > > > > >> > > > >>> > > > > > >> > > > /* A string or integer constant (one must know >>> which from >>> > > > > > context). */ >>> > > > > > >> > > > -union expr_constant { >>> > > > > > >> > > > - /* Integer constant. >>> > > > > > >> > > > - * >>> > > > > > >> > > > - * The width of a constant isn't always clear, >>> e.g. >>> > if >>> > > > > > you write "1", >>> > > > > > >> > > > - * there's no way to tell whether you mean for >>> that >>> > to be >>> > > > > > a 1-bit constant >>> > > > > > >> > > > - * or a 128-bit constant or somewhere in >>> between. */ >>> > > > > > >> > > > - struct { >>> > > > > > >> > > > - union mf_subvalue value; >>> > > > > > >> > > > - union mf_subvalue mask; /* Only initialized >>> if >>> > > > > > 'masked'. */ >>> > > > > > >> > > > - bool masked; >>> > > > > > >> > > > - >>> > > > > > >> > > > - enum lex_format format; /* From the >>> constant's >>> > > > > > lex_token. */ >>> > > > > > >> > > > - }; >>> > > > > > >> > > > +struct expr_constant { >>> > > > > > >> > > > + const char *as_name; >>> > > > > > >> > > > >>> > > > > > >> > > > - /* Null-terminated string constant. */ >>> > > > > > >> > > > - char *string; >>> > > > > > >> > > > + union { >>> > > > > > >> > > > + /* Integer constant. >>> > > > > > >> > > > + * >>> > > > > > >> > > > + * The width of a constant isn't always >>> clear, >>> > e.g. >>> > > > > > if you write "1", >>> > > > > > >> > > > + * there's no way to tell whether you mean >>> for >>> > that >>> > > > > > to be a 1-bit >>> > > > > > >> > > > + * constant or a 128-bit constant or >>> somewhere in >>> > > > > > between. */ >>> > > > > > >> > > > + struct { >>> > > > > > >> > > > + union mf_subvalue value; >>> > > > > > >> > > > + union mf_subvalue mask; /* Only >>> initialized >>> > if >>> > > > > > 'masked'. */ >>> > > > > > >> > > > + bool masked; >>> > > > > > >> > > > + >>> > > > > > >> > > > + enum lex_format format; /* From the >>> > constant's >>> > > > > > lex_token. */ >>> > > > > > >> > > > + }; >>> > > > > > >> > > > + >>> > > > > > >> > > > + /* Null-terminated string constant. */ >>> > > > > > >> > > > + char *string; >>> > > > > > >> > > > + }; >>> > > > > > >> > > > }; >>> > > > > > >> > > > >>> > > > > > >> > > > bool expr_constant_parse(struct lexer *, >>> > > > > > >> > > > const struct expr_field *, >>> > > > > > >> > > > - union expr_constant *); >>> > > > > > >> > > > -void expr_constant_format(const union expr_constant >>> *, >>> > > > > > >> > > > + struct expr_constant *); >>> > > > > > >> > > > +void expr_constant_format(const struct >>> expr_constant *, >>> > > > > > >> > > > enum expr_constant_type, >>> > struct ds >>> > > > *); >>> > > > > > >> > > > -void expr_constant_destroy(const union >>> expr_constant *, >>> > > > > > >> > > > +void expr_constant_destroy(const struct >>> expr_constant *, >>> > > > > > >> > > > enum expr_constant_type); >>> > > > > > >> > > > >>> > > > > > >> > > > /* A collection of "union expr_constant"s of the >>> same >>> > type. >>> > > > */ >>> > > > > > >> > > > struct expr_constant_set { >>> > > > > > >> > > > - union expr_constant *values; /* Constants. */ >>> > > > > > >> > > > + struct expr_constant *values; /* Constants. */ >>> > > > > > >> > > > size_t n_values; /* Number of >>> > constants. */ >>> > > > > > >> > > > enum expr_constant_type type; /* Type of the >>> > constants. >>> > > > */ >>> > > > > > >> > > > bool in_curlies; /* Whether the >>> > constants >>> > > > > > were in {}. */ >>> > > > > > >> > > > - char *as_name; /* Name of an >>> address >>> > set. >>> > > > > > It is NULL if not >>> > > > > > >> > > > - an address >>> set. */ >>> > > > > > >> > > > }; >>> > > > > > >> > > > >>> > > > > > >> > > > bool expr_constant_set_parse(struct lexer *, struct >>> > > > > > expr_constant_set *); >>> > > > > > >> > > > diff --git a/lib/actions.c b/lib/actions.c >>> > > > > > >> > > > index 94751d059..cff4f9e3c 100644 >>> > > > > > >> > > > --- a/lib/actions.c >>> > > > > > >> > > > +++ b/lib/actions.c >>> > > > > > >> > > > @@ -450,7 +450,7 @@ encode_LOAD(const struct >>> ovnact_load >>> > > > *load, >>> > > > > > >> > > > const struct ovnact_encode_params *ep, >>> > > > > > >> > > > struct ofpbuf *ofpacts) >>> > > > > > >> > > > { >>> > > > > > >> > > > - const union expr_constant *c = &load->imm; >>> > > > > > >> > > > + const struct expr_constant *c = &load->imm; >>> > > > > > >> > > > struct mf_subfield dst = >>> > expr_resolve_field(&load->dst); >>> > > > > > >> > > > struct ofpact_set_field *sf = >>> > > > > > ofpact_put_set_field(ofpacts, dst.field, >>> > > > > > >> > > > >>> > NULL, >>> > > > > > NULL); >>> > > > > > >> > > > @@ -2077,7 +2077,7 @@ >>> > > > > > encode_event_empty_lb_backends_opts(struct ofpbuf *ofpacts, >>> > > > > > >> > > > /* All empty_lb_backends fields are of type >>> > 'str' */ >>> > > > > > >> > > > ovs_assert(!strcmp(o->option->type, "str")); >>> > > > > > >> > > > >>> > > > > > >> > > > - const union expr_constant *c = >>> o->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = >>> o->value.values; >>> > > > > > >> > > > size_t size = strlen(c->string); >>> > > > > > >> > > > struct controller_event_opt_header hdr = >>> > > > > > >> > > > (struct controller_event_opt_header) { >>> > > > > > >> > > > @@ -2553,7 +2553,7 @@ >>> validate_empty_lb_backends(struct >>> > > > > > action_context *ctx, >>> > > > > > >> > > > { >>> > > > > > >> > > > for (size_t i = 0; i < n_options; i++) { >>> > > > > > >> > > > const struct ovnact_gen_option *o = >>> &options[i]; >>> > > > > > >> > > > - const union expr_constant *c = >>> o->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = >>> o->value.values; >>> > > > > > >> > > > struct sockaddr_storage ss; >>> > > > > > >> > > > struct uuid uuid; >>> > > > > > >> > > > >>> > > > > > >> > > > @@ -2861,7 +2861,7 @@ encode_put_dhcpv4_option(const >>> > struct >>> > > > > > ovnact_gen_option *o, >>> > > > > > >> > > > uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, >>> 2); >>> > > > > > >> > > > opt_header[0] = o->option->code; >>> > > > > > >> > > > >>> > > > > > >> > > > - const union expr_constant *c = o->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = o->value.values; >>> > > > > > >> > > > size_t n_values = o->value.n_values; >>> > > > > > >> > > > if (!strcmp(o->option->type, "bool") || >>> > > > > > >> > > > !strcmp(o->option->type, "uint8")) { >>> > > > > > >> > > > @@ -3027,7 +3027,7 @@ encode_put_dhcpv6_option(const >>> > struct >>> > > > > > ovnact_gen_option *o, >>> > > > > > >> > > > struct ofpbuf *ofpacts) >>> > > > > > >> > > > { >>> > > > > > >> > > > struct dhcpv6_opt_header *opt = >>> > > > > > ofpbuf_put_uninit(ofpacts, sizeof *opt); >>> > > > > > >> > > > - const union expr_constant *c = o->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = o->value.values; >>> > > > > > >> > > > size_t n_values = o->value.n_values; >>> > > > > > >> > > > size_t size; >>> > > > > > >> > > > >>> > > > > > >> > > > @@ -3083,7 +3083,7 @@ encode_PUT_DHCPV4_OPTS(const >>> struct >>> > > > > > ovnact_put_opts *pdo, >>> > > > > > >> > > > find_opt(pdo->options, pdo->n_options, >>> > > > > > DHCP_OPT_BOOTFILE_CODE); >>> > > > > > >> > > > if (boot_opt) { >>> > > > > > >> > > > uint8_t *opt_header = >>> ofpbuf_put_zeros(ofpacts, >>> > 2); >>> > > > > > >> > > > - const union expr_constant *c = >>> > > > boot_opt->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = >>> > > > boot_opt->value.values; >>> > > > > > >> > > > opt_header[0] = boot_opt->option->code; >>> > > > > > >> > > > opt_header[1] = strlen(c->string); >>> > > > > > >> > > > ofpbuf_put(ofpacts, c->string, >>> opt_header[1]); >>> > > > > > >> > > > @@ -3093,7 +3093,7 @@ encode_PUT_DHCPV4_OPTS(const >>> struct >>> > > > > > ovnact_put_opts *pdo, >>> > > > > > >> > > > find_opt(pdo->options, pdo->n_options, >>> > > > > > DHCP_OPT_BOOTFILE_ALT_CODE); >>> > > > > > >> > > > if (boot_alt_opt) { >>> > > > > > >> > > > uint8_t *opt_header = >>> ofpbuf_put_zeros(ofpacts, >>> > 2); >>> > > > > > >> > > > - const union expr_constant *c = >>> > > > > > boot_alt_opt->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = >>> > > > > > boot_alt_opt->value.values; >>> > > > > > >> > > > opt_header[0] = boot_alt_opt->option->code; >>> > > > > > >> > > > opt_header[1] = strlen(c->string); >>> > > > > > >> > > > ofpbuf_put(ofpacts, c->string, >>> opt_header[1]); >>> > > > > > >> > > > @@ -3103,7 +3103,7 @@ encode_PUT_DHCPV4_OPTS(const >>> struct >>> > > > > > ovnact_put_opts *pdo, >>> > > > > > >> > > > pdo->options, pdo->n_options, >>> > > > DHCP_OPT_NEXT_SERVER_CODE); >>> > > > > > >> > > > if (next_server_opt) { >>> > > > > > >> > > > uint8_t *opt_header = >>> ofpbuf_put_zeros(ofpacts, >>> > 2); >>> > > > > > >> > > > - const union expr_constant *c = >>> > > > > > next_server_opt->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = >>> > > > > > next_server_opt->value.values; >>> > > > > > >> > > > opt_header[0] = >>> next_server_opt->option->code; >>> > > > > > >> > > > opt_header[1] = sizeof(ovs_be32); >>> > > > > > >> > > > ofpbuf_put(ofpacts, &c->value.ipv4, >>> > > > sizeof(ovs_be32)); >>> > > > > > >> > > > @@ -3307,7 +3307,7 @@ parse_put_nd_ra_opts(struct >>> > > > > > action_context *ctx, const struct expr_field *dst, >>> > > > > > >> > > > /* Let's validate the options. */ >>> > > > > > >> > > > for (size_t i = 0; i < po->n_options; i++) { >>> > > > > > >> > > > const struct ovnact_gen_option *o = >>> > &po->options[i]; >>> > > > > > >> > > > - const union expr_constant *c = >>> o->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = >>> o->value.values; >>> > > > > > >> > > > if (o->value.n_values > 1) { >>> > > > > > >> > > > lexer_error(ctx->lexer, "Invalid value >>> for >>> > \"%s\" >>> > > > > > option", >>> > > > > > >> > > > o->option->name); >>> > > > > > >> > > > @@ -3490,7 +3490,7 @@ static void >>> > > > > > >> > > > encode_put_nd_ra_option(const struct >>> ovnact_gen_option >>> > *o, >>> > > > > > >> > > > struct ofpbuf *ofpacts, >>> ptrdiff_t >>> > > > > > ra_offset) >>> > > > > > >> > > > { >>> > > > > > >> > > > - const union expr_constant *c = o->value.values; >>> > > > > > >> > > > + const struct expr_constant *c = o->value.values; >>> > > > > > >> > > > >>> > > > > > >> > > > switch (o->option->code) { >>> > > > > > >> > > > case ND_RA_FLAG_ADDR_MODE: >>> > > > > > >> > > > diff --git a/lib/expr.c b/lib/expr.c >>> > > > > > >> > > > index 0cb44e1b6..ecf8a6338 100644 >>> > > > > > >> > > > --- a/lib/expr.c >>> > > > > > >> > > > +++ b/lib/expr.c >>> > > > > > >> > > > @@ -179,12 +179,10 @@ expr_combine(enum expr_type >>> type, >>> > struct >>> > > > > > expr *a, struct expr *b) >>> > > > > > >> > > > } else { >>> > > > > > >> > > > ovs_list_push_back(&a->andor, &b->node); >>> > > > > > >> > > > } >>> > > > > > >> > > > - free(a->as_name); >>> > > > > > >> > > > a->as_name = NULL; >>> > > > > > >> > > > return a; >>> > > > > > >> > > > } else if (b->type == type) { >>> > > > > > >> > > > ovs_list_push_front(&b->andor, &a->node); >>> > > > > > >> > > > - free(b->as_name); >>> > > > > > >> > > > b->as_name = NULL; >>> > > > > > >> > > > return b; >>> > > > > > >> > > > } else { >>> > > > > > >> > > > @@ -521,12 +519,13 @@ static bool parse_field(struct >>> > > > > > expr_context *, struct expr_field *); >>> > > > > > >> > > > >>> > > > > > >> > > > static struct expr * >>> > > > > > >> > > > make_cmp__(const struct expr_field *f, enum >>> expr_relop r, >>> > > > > > >> > > > - const union expr_constant *c) >>> > > > > > >> > > > + const struct expr_constant *c) >>> > > > > > >> > > > { >>> > > > > > >> > > > struct expr *e = xzalloc(sizeof *e); >>> > > > > > >> > > > e->type = EXPR_T_CMP; >>> > > > > > >> > > > e->cmp.symbol = f->symbol; >>> > > > > > >> > > > e->cmp.relop = r; >>> > > > > > >> > > > + e->as_name = c->as_name; >>> > > > > > >> > > > if (f->symbol->width) { >>> > > > > > >> > > > bitwise_copy(&c->value, sizeof c->value, 0, >>> > > > > > >> > > > &e->cmp.value, sizeof >>> e->cmp.value, >>> > > > f->ofs, >>> > > > > > >> > > > @@ -547,7 +546,7 @@ make_cmp__(const struct >>> expr_field *f, >>> > > > > > enum expr_relop r, >>> > > > > > >> > > > >>> > > > > > >> > > > /* Returns the minimum reasonable width for integer >>> > constant >>> > > > > > 'c'. */ >>> > > > > > >> > > > static int >>> > > > > > >> > > > -expr_constant_width(const union expr_constant *c) >>> > > > > > >> > > > +expr_constant_width(const struct expr_constant *c) >>> > > > > > >> > > > { >>> > > > > > >> > > > if (c->masked) { >>> > > > > > >> > > > return mf_subvalue_width(&c->mask); >>> > > > > > >> > > > @@ -674,10 +673,7 @@ make_cmp(struct expr_context >>> *ctx, >>> > > > > > >> > > > e = expr_combine(r == EXPR_R_EQ ? EXPR_T_OR >>> : >>> > > > EXPR_T_AND, >>> > > > > > >> > > > e, make_cmp__(f, r, >>> > > > &cs->values[i])); >>> > > > > > >> > > > } >>> > > > > > >> > > > - /* Track address set */ >>> > > > > > >> > > > - if (r == EXPR_R_EQ && e->type == EXPR_T_OR && >>> > > > cs->as_name) { >>> > > > > > >> > > > - e->as_name = xstrdup(cs->as_name); >>> > > > > > >> > > > - } >>> > > > > > >> > > > + >>> > > > > > >> > > > exit: >>> > > > > > >> > > > expr_constant_set_destroy(cs); >>> > > > > > >> > > > return e; >>> > > > > > >> > > > @@ -797,11 +793,10 @@ parse_addr_sets(struct >>> expr_context >>> > > > > > *ctx, struct expr_constant_set *cs, >>> > > > > > >> > > > } >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - struct expr_constant_set *addr_sets >>> > > > > > >> > > > - = (ctx->addr_sets >>> > > > > > >> > > > - ? shash_find_data(ctx->addr_sets, >>> > > > ctx->lexer->token.s) >>> > > > > > >> > > > - : NULL); >>> > > > > > >> > > > - if (!addr_sets) { >>> > > > > > >> > > > + struct shash_node *node = ctx->addr_sets >>> > > > > > >> > > > + ? >>> > shash_find(ctx->addr_sets, >>> > > > > > ctx->lexer->token.s) >>> > > > > > >> > > > + : NULL; >>> > > > > > >> > > > + if (!node) { >>> > > > > > >> > > > lexer_syntax_error(ctx->lexer, "expecting >>> > address set >>> > > > > > name"); >>> > > > > > >> > > > return false; >>> > > > > > >> > > > } >>> > > > > > >> > > > @@ -810,17 +805,16 @@ parse_addr_sets(struct >>> expr_context >>> > > > > > *ctx, struct expr_constant_set *cs, >>> > > > > > >> > > > return false; >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - if (!cs->n_values) { >>> > > > > > >> > > > - cs->as_name = xstrdup(ctx->lexer->token.s); >>> > > > > > >> > > > - } >>> > > > > > >> > > > - >>> > > > > > >> > > > + struct expr_constant_set *addr_sets = >>> node->data; >>> > > > > > >> > > > size_t n_values = cs->n_values + >>> addr_sets->n_values; >>> > > > > > >> > > > if (n_values >= *allocated_values) { >>> > > > > > >> > > > cs->values = xrealloc(cs->values, n_values * >>> > sizeof >>> > > > > > *cs->values); >>> > > > > > >> > > > *allocated_values = n_values; >>> > > > > > >> > > > } >>> > > > > > >> > > > for (size_t i = 0; i < addr_sets->n_values; >>> i++) { >>> > > > > > >> > > > - cs->values[cs->n_values++] = >>> > addr_sets->values[i]; >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > + *c = addr_sets->values[i]; >>> > > > > > >> > > > + c->as_name = node->name; >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > return true; >>> > > > > > >> > > > @@ -859,8 +853,9 @@ parse_port_group(struct >>> expr_context >>> > *ctx, >>> > > > > > struct expr_constant_set *cs, >>> > > > > > >> > > > *allocated_values = n_values; >>> > > > > > >> > > > } >>> > > > > > >> > > > for (size_t i = 0; i < port_group->n_values; >>> i++) { >>> > > > > > >> > > > - cs->values[cs->n_values++].string = >>> > > > > > >> > > > - xstrdup(port_group->values[i].string); >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > + c->string = >>> > xstrdup(port_group->values[i].string); >>> > > > > > >> > > > + c->as_name = NULL; >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > return true; >>> > > > > > >> > > > @@ -875,11 +870,6 @@ parse_constant(struct >>> expr_context >>> > *ctx, >>> > > > > > struct expr_constant_set *cs, >>> > > > > > >> > > > sizeof *cs->values); >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - /* Combining other values to the constant set >>> that is >>> > > > > > tracking an >>> > > > > > >> > > > - * address set, so untrack it. */ >>> > > > > > >> > > > - free(cs->as_name); >>> > > > > > >> > > > - cs->as_name = NULL; >>> > > > > > >> > > > - >>> > > > > > >> > > > if (ctx->lexer->token.type == LEX_T_TEMPLATE) { >>> > > > > > >> > > > lexer_error(ctx->lexer, "Unexpanded >>> template."); >>> > > > > > >> > > > return false; >>> > > > > > >> > > > @@ -887,7 +877,9 @@ parse_constant(struct >>> expr_context >>> > *ctx, >>> > > > > > struct expr_constant_set *cs, >>> > > > > > >> > > > if (!assign_constant_set_type(ctx, cs, >>> > > > EXPR_C_STRING)) { >>> > > > > > >> > > > return false; >>> > > > > > >> > > > } >>> > > > > > >> > > > - cs->values[cs->n_values++].string = >>> > > > > > xstrdup(ctx->lexer->token.s); >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > + c->string = xstrdup(ctx->lexer->token.s); >>> > > > > > >> > > > + c->as_name = NULL; >>> > > > > > >> > > > lexer_get(ctx->lexer); >>> > > > > > >> > > > return true; >>> > > > > > >> > > > } else if (ctx->lexer->token.type == >>> LEX_T_INTEGER || >>> > > > > > >> > > > @@ -896,13 +888,14 @@ parse_constant(struct >>> expr_context >>> > *ctx, >>> > > > > > struct expr_constant_set *cs, >>> > > > > > >> > > > return false; >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - union expr_constant *c = >>> > &cs->values[cs->n_values++]; >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > c->value = ctx->lexer->token.value; >>> > > > > > >> > > > c->format = ctx->lexer->token.format; >>> > > > > > >> > > > c->masked = ctx->lexer->token.type == >>> > > > > > LEX_T_MASKED_INTEGER; >>> > > > > > >> > > > if (c->masked) { >>> > > > > > >> > > > c->mask = ctx->lexer->token.mask; >>> > > > > > >> > > > } >>> > > > > > >> > > > + c->as_name = NULL; >>> > > > > > >> > > > lexer_get(ctx->lexer); >>> > > > > > >> > > > return true; >>> > > > > > >> > > > } else if (ctx->lexer->token.type == >>> LEX_T_MACRO) { >>> > > > > > >> > > > @@ -961,7 +954,7 @@ parse_constant_set(struct >>> expr_context >>> > > > > > *ctx, struct expr_constant_set *cs) >>> > > > > > >> > > > * indeterminate. */ >>> > > > > > >> > > > bool >>> > > > > > >> > > > expr_constant_parse(struct lexer *lexer, const >>> struct >>> > > > > > expr_field *f, >>> > > > > > >> > > > - union expr_constant *c) >>> > > > > > >> > > > + struct expr_constant *c) >>> > > > > > >> > > > { >>> > > > > > >> > > > if (lexer->error) { >>> > > > > > >> > > > return false; >>> > > > > > >> > > > @@ -987,7 +980,7 @@ expr_constant_parse(struct lexer >>> > *lexer, >>> > > > > > const struct expr_field *f, >>> > > > > > >> > > > /* Appends to 's' a re-parseable representation of >>> > constant >>> > > > > > 'c' with the given >>> > > > > > >> > > > * 'type'. */ >>> > > > > > >> > > > void >>> > > > > > >> > > > -expr_constant_format(const union expr_constant *c, >>> > > > > > >> > > > +expr_constant_format(const struct expr_constant *c, >>> > > > > > >> > > > enum expr_constant_type type, >>> > struct ds >>> > > > *s) >>> > > > > > >> > > > { >>> > > > > > >> > > > if (type == EXPR_C_STRING) { >>> > > > > > >> > > > @@ -1010,7 +1003,7 @@ expr_constant_format(const >>> union >>> > > > > > expr_constant *c, >>> > > > > > >> > > > * >>> > > > > > >> > > > * Does not free(c). */ >>> > > > > > >> > > > void >>> > > > > > >> > > > -expr_constant_destroy(const union expr_constant *c, >>> > > > > > >> > > > +expr_constant_destroy(const struct expr_constant *c, >>> > > > > > >> > > > enum expr_constant_type type) >>> > > > > > >> > > > { >>> > > > > > >> > > > if (c && type == EXPR_C_STRING) { >>> > > > > > >> > > > @@ -1043,7 +1036,7 @@ expr_constant_set_format(const >>> > struct >>> > > > > > expr_constant_set *cs, struct ds *s) >>> > > > > > >> > > > ds_put_char(s, '{'); >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - for (const union expr_constant *c = cs->values; >>> > > > > > >> > > > + for (const struct expr_constant *c = cs->values; >>> > > > > > >> > > > c < &cs->values[cs->n_values]; c++) { >>> > > > > > >> > > > if (c != cs->values) { >>> > > > > > >> > > > ds_put_cstr(s, ", "); >>> > > > > > >> > > > @@ -1067,15 +1060,14 @@ >>> expr_constant_set_destroy(struct >>> > > > > > expr_constant_set *cs) >>> > > > > > >> > > > } >>> > > > > > >> > > > } >>> > > > > > >> > > > free(cs->values); >>> > > > > > >> > > > - free(cs->as_name); >>> > > > > > >> > > > } >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > static int >>> > > > > > >> > > > compare_expr_constant_integer_cb(const void *a_, >>> const >>> > void >>> > > > *b_) >>> > > > > > >> > > > { >>> > > > > > >> > > > - const union expr_constant *a = a_; >>> > > > > > >> > > > - const union expr_constant *b = b_; >>> > > > > > >> > > > + const struct expr_constant *a = a_; >>> > > > > > >> > > > + const struct expr_constant *b = b_; >>> > > > > > >> > > > >>> > > > > > >> > > > int d = memcmp(&a->value, &b->value, sizeof >>> > a->value); >>> > > > > > >> > > > if (d) { >>> > > > > > >> > > > @@ -1114,7 +1106,7 @@ >>> > expr_constant_set_create_integers(const >>> > > > > > char *const *values, size_t n_values) >>> > > > > > >> > > > VLOG_WARN("Invalid constant set entry: >>> '%s', >>> > > > > > token type: %d", >>> > > > > > >> > > > values[i], lex.token.type); >>> > > > > > >> > > > } else { >>> > > > > > >> > > > - union expr_constant *c = >>> > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > c->value = lex.token.value; >>> > > > > > >> > > > c->format = lex.token.format; >>> > > > > > >> > > > c->masked = lex.token.type == >>> > > > LEX_T_MASKED_INTEGER; >>> > > > > > >> > > > @@ -1135,7 +1127,7 @@ >>> > expr_constant_set_create_integers(const >>> > > > > > char *const *values, size_t n_values) >>> > > > > > >> > > > >>> > > > > > >> > > > static void >>> > > > > > >> > > > expr_constant_set_add_value(struct expr_constant_set >>> > **p_cs, >>> > > > > > >> > > > - union expr_constant *c, >>> > size_t >>> > > > > > *allocated) >>> > > > > > >> > > > + struct expr_constant *c, >>> > size_t >>> > > > > > *allocated) >>> > > > > > >> > > > { >>> > > > > > >> > > > struct expr_constant_set *cs = *p_cs; >>> > > > > > >> > > > if (!cs) { >>> > > > > > >> > > > @@ -1246,7 +1238,7 @@ >>> expr_const_sets_add_strings(struct >>> > shash >>> > > > > > *const_sets, const char *name, >>> > > > > > >> > > > values[i], name); >>> > > > > > >> > > > continue; >>> > > > > > >> > > > } >>> > > > > > >> > > > - union expr_constant *c = >>> > &cs->values[cs->n_values++]; >>> > > > > > >> > > > + struct expr_constant *c = >>> > > > &cs->values[cs->n_values++]; >>> > > > > > >> > > > c->string = xstrdup(values[i]); >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > @@ -1359,7 +1351,7 @@ expr_parse_primary(struct >>> > expr_context >>> > > > > > *ctx, bool *atomic) >>> > > > > > >> > > > >>> > > > > > >> > > > *atomic = true; >>> > > > > > >> > > > >>> > > > > > >> > > > - union expr_constant *cst = >>> xzalloc(sizeof >>> > *cst); >>> > > > > > >> > > > + struct expr_constant *cst = >>> xzalloc(sizeof >>> > *cst); >>> > > > > > >> > > > cst->format = LEX_F_HEXADECIMAL; >>> > > > > > >> > > > cst->masked = false; >>> > > > > > >> > > > >>> > > > > > >> > > > @@ -1367,7 +1359,6 @@ expr_parse_primary(struct >>> > expr_context >>> > > > > > *ctx, bool *atomic) >>> > > > > > >> > > > c.values = cst; >>> > > > > > >> > > > c.n_values = 1; >>> > > > > > >> > > > c.in_curlies = false; >>> > > > > > >> > > > - c.as_name = NULL; >>> > > > > > >> > > > return make_cmp(ctx, &f, EXPR_R_NE, &c); >>> > > > > > >> > > > } else if (parse_relop(ctx, &r) && >>> > > > > > parse_constant_set(ctx, &c)) { >>> > > > > > >> > > > return make_cmp(ctx, &f, r, &c); >>> > > > > > >> > > > @@ -1834,7 +1825,6 @@ expr_symtab_destroy(struct >>> shash >>> > > > *symtab) >>> > > > > > >> > > > static struct expr * >>> > > > > > >> > > > expr_clone_cmp(struct expr *expr) >>> > > > > > >> > > > { >>> > > > > > >> > > > - ovs_assert(!expr->as_name); >>> > > > > > >> > > > struct expr *new = xmemdup(expr, sizeof *expr); >>> > > > > > >> > > > if (!new->cmp.symbol->width) { >>> > > > > > >> > > > new->cmp.string = xstrdup(new->cmp.string); >>> > > > > > >> > > > @@ -1858,7 +1848,6 @@ expr_clone_andor(struct expr >>> *expr) >>> > > > > > >> > > > static struct expr * >>> > > > > > >> > > > expr_clone_condition(struct expr *expr) >>> > > > > > >> > > > { >>> > > > > > >> > > > - ovs_assert(!expr->as_name); >>> > > > > > >> > > > struct expr *new = xmemdup(expr, sizeof *expr); >>> > > > > > >> > > > new->cond.string = xstrdup(new->cond.string); >>> > > > > > >> > > > return new; >>> > > > > > >> > > > @@ -1894,8 +1883,6 @@ expr_destroy(struct expr *expr) >>> > > > > > >> > > > return; >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - free(expr->as_name); >>> > > > > > >> > > > - >>> > > > > > >> > > > struct expr *sub; >>> > > > > > >> > > > >>> > > > > > >> > > > switch (expr->type) { >>> > > > > > >> > > > @@ -2567,7 +2554,7 @@ crush_or_supersets(struct expr >>> > *expr, >>> > > > > > const struct expr_symbol *symbol) >>> > > > > > >> > > > * mask sizes. */ >>> > > > > > >> > > > size_t n = ovs_list_size(&expr->andor); >>> > > > > > >> > > > struct expr **subs = xmalloc(n * sizeof *subs); >>> > > > > > >> > > > - bool modified = false; >>> > > > > > >> > > > + bool has_addr_set = false; >>> > > > > > >> > > > /* Linked list over the 'subs' array to quickly >>> skip >>> > > > > > deleted elements, >>> > > > > > >> > > > * i.e. the index of the next potentially >>> non-NULL >>> > > > > > element. */ >>> > > > > > >> > > > size_t *next = xmalloc(n * sizeof *next); >>> > > > > > >> > > > @@ -2575,6 +2562,9 @@ crush_or_supersets(struct expr >>> > *expr, >>> > > > > > const struct expr_symbol *symbol) >>> > > > > > >> > > > size_t i = 0, j, max_n_bits = 0; >>> > > > > > >> > > > struct expr *sub; >>> > > > > > >> > > > LIST_FOR_EACH (sub, node, &expr->andor) { >>> > > > > > >> > > > + if (sub->as_name) { >>> > > > > > >> > > > + has_addr_set = true; >>> > > > > > >> > > > + } >>> > > > > > >> > > > if (symbol->width) { >>> > > > > > >> > > > const unsigned long *sub_mask; >>> > > > > > >> > > > >>> > > > > > >> > > > @@ -2596,14 +2586,14 @@ crush_or_supersets(struct >>> expr >>> > *expr, >>> > > > > > const struct expr_symbol *symbol) >>> > > > > > >> > > > next[last] = i; >>> > > > > > >> > > > last = i; >>> > > > > > >> > > > } else { >>> > > > > > >> > > > + /* Remove address set reference from the >>> > > > > > duplicate. */ >>> > > > > > >> > > > + subs[last]->as_name = NULL; >>> > > > > > >> > > > expr_destroy(subs[i]); >>> > > > > > >> > > > subs[i] = NULL; >>> > > > > > >> > > > - modified = true; >>> > > > > > >> > > > } >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - if (!symbol->width || symbol->level != >>> EXPR_L_ORDINAL >>> > > > > > >> > > > - || (!modified && expr->as_name)) { >>> > > > > > >> > > > + if (!symbol->width || symbol->level != >>> > EXPR_L_ORDINAL || >>> > > > > > has_addr_set) { >>> > > > > > >> > > > /* Not a fully maskable field or this >>> expression >>> > is >>> > > > > > tracking an >>> > > > > > >> > > > * address set. Don't try to optimize to >>> > preserve >>> > > > > > address set I-P. */ >>> > > > > > >> > > > goto done; >>> > > > > > >> > > > @@ -2658,10 +2648,11 @@ crush_or_supersets(struct >>> expr >>> > *expr, >>> > > > > > const struct expr_symbol *symbol) >>> > > > > > >> > > > if (expr_bitmap_intersect_check(a_value, >>> > a_mask, >>> > > > > > b_value, b_mask, >>> > > > > > >> > > > >>> bit_width) >>> > > > > > >> > > > && bitmap_is_superset(b_mask, >>> a_mask, >>> > > > > > bit_width)) { >>> > > > > > >> > > > - /* 'a' is the same expression with a >>> > smaller >>> > > > > > mask. */ >>> > > > > > >> > > > + /* 'a' is the same expression with a >>> > smaller >>> > > > > > mask. >>> > > > > > >> > > > + * Remove address set reference >>> from the >>> > > > > > duplicate. */ >>> > > > > > >> > > > + a->as_name = NULL; >>> > > > > > >> > > > expr_destroy(subs[j]); >>> > > > > > >> > > > subs[j] = NULL; >>> > > > > > >> > > > - modified = true; >>> > > > > > >> > > > >>> > > > > > >> > > > /* Shorten the path for the next >>> round. >>> > */ >>> > > > > > >> > > > if (last) { >>> > > > > > >> > > > @@ -2685,12 +2676,6 @@ done: >>> > > > > > >> > > > } >>> > > > > > >> > > > } >>> > > > > > >> > > > >>> > > > > > >> > > > - if (modified) { >>> > > > > > >> > > > - /* Members modified, so untrack address >>> set. */ >>> > > > > > >> > > > - free(expr->as_name); >>> > > > > > >> > > > - expr->as_name = NULL; >>> > > > > > >> > > > - } >>> > > > > > >> > > > - >>> > > > > > >> > > > free(next); >>> > > > > > >> > > > free(subs); >>> > > > > > >> > > > return expr; >>> > > > > > >> > > > @@ -3271,10 +3256,10 @@ add_disjunction(const struct >>> expr >>> > *or, >>> > > > > > >> > > > LIST_FOR_EACH (sub, node, &or->andor) { >>> > > > > > >> > > > struct expr_match *match = expr_match_new(m, >>> > clause, >>> > > > > > n_clauses, >>> > > > > > >> > > > >>> > conj_id); >>> > > > > > >> > > > - if (or->as_name) { >>> > > > > > >> > > > + if (sub->as_name) { >>> > > > > > >> > > > ovs_assert(sub->type == EXPR_T_CMP); >>> > > > > > >> > > > ovs_assert(sub->cmp.symbol->width); >>> > > > > > >> > > > - match->as_name = xstrdup(or->as_name); >>> > > > > > >> > > > + match->as_name = xstrdup(sub->as_name); >>> > > > > > >> > > > match->as_ip = sub->cmp.value.ipv6; >>> > > > > > >> > > > match->as_mask = sub->cmp.mask.ipv6; >>> > > > > > >> > > > } >>> > > > > > >> > > > diff --git a/tests/ovn-controller.at >>> > > > > > <http://ovn-controller.at> b/tests/ovn-controller.at >>> > > > > > <http://ovn-controller.at> >>> > > > > > >> > > > index be198e00d..27cec2aec 100644 >>> > > > > > >> > > > --- a/tests/ovn-controller.at < >>> http://ovn-controller.at> >>> > > > > > >> > > > +++ b/tests/ovn-controller.at < >>> http://ovn-controller.at> >>> > > > > > >> > > > @@ -1625,7 +1625,9 @@ >>> > > > > > >>> > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.8 >>> > > > > > actions=lo >>> > > > > > >> > > > done >>> > > > > > >> > > > >>> > > > > > >> > > > reprocess_count_new=$(read_counter >>> consider_logical_flow) >>> > > > > > >> > > > -AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [10 >>> > > > > > >> > > > +# First 2 reprocess are due to change from 1 IP in >>> AS to >>> > 2. >>> > > > > > >> > > > +# Last 5 is due to overlap in IP addresses between >>> as1 >>> > and >>> > > > as2. >>> > > > > > >> > > > +AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [7 >>> > > > > > >> > > > ]) >>> > > > > > >> > > > >>> > > > > > >> > > > # Remove the IPs from as1 and as2, 1 IP each time. >>> > > > > > >> > > > @@ -1648,9 +1650,9 @@ for i in $(seq 10); do >>> > > > > > >> > > > done >>> > > > > > >> > > > >>> > > > > > >> > > > reprocess_count_new=$(read_counter >>> consider_logical_flow) >>> > > > > > >> > > > -# In this case the as1 and as2 are merged to a >>> single OR >>> > > > > > expr, so we lose track of >>> > > > > > >> > > > -# address set information - can't handle deletion >>> > > > incrementally. >>> > > > > > >> > > > -AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [10 >>> > > > > > >> > > > +# First 5 are due to overlap in IP addresses >>> between as1 >>> > and >>> > > > as2. >>> > > > > > >> > > > +# Last 2 are due to change from 2 IP in AS to 1. >>> > > > > > >> > > > +AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [7 >>> > > > > > >> > > > ]) >>> > > > > > >> > > > >>> > > > > > >> > > > OVN_CLEANUP([hv1]) >>> > > > > > >> > > > @@ -1817,7 +1819,7 @@ >>> > > > > > >>> > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.5 >>> > > > > > actions=co >>> > > > > > >> > > > >>> > > > > > >>> > > > >>> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 >>> > > > actions=conjunction,2/2) >>> > > > > > >> > > > ]) >>> > > > > > >> > > > reprocess_count_new=$(read_counter >>> consider_logical_flow) >>> > > > > > >> > > > -AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [1 >>> > > > > > >> > > > +AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [0 >>> > > > > > >> > > > ]) >>> > > > > > >> > > > >>> > > > > > >> > > > # Delete 2 IPs >>> > > > > > >> > > > @@ -1837,7 +1839,7 @@ >>> > > > > > >>> > priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.0.0.3 >>> > > > > > actions=co >>> > > > > > >> > > > >>> > > > > > >>> > > > >>> priority=1100,ip,reg15=0x$port_key,metadata=0x$dp_key,nw_src=10.10.10.10 >>> > > > actions=conjunction,2/2) >>> > > > > > >> > > > ]) >>> > > > > > >> > > > reprocess_count_new=$(read_counter >>> consider_logical_flow) >>> > > > > > >> > > > -AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [1 >>> > > > > > >> > > > +AT_CHECK([echo $(($reprocess_count_new - >>> > > > > > $reprocess_count_old))], [0], [0 >>> > > > > > >> > > > ]) >>> > > > > > >> > > > >>> > > > > > >> > > > OVN_CLEANUP([hv1]) >>> > > > > > >> > > > @@ -2906,3 +2908,68 @@ OVN_CLEANUP([hv1],[hv2]) >>> > > > > > >> > > > >>> > > > > > >> > > > AT_CLEANUP >>> > > > > > >> > > > ]) >>> > > > > > >> > > > + >>> > > > > > >> > > > +AT_SETUP([ovn-controller - AS I-P and recompute >>> > consistency]) >>> > > > > > >> > > > +AT_KEYWORDS([as-i-p]) >>> > > > > > >> > > > + >>> > > > > > >> > > > +ovn_start >>> > > > > > >> > > > + >>> > > > > > >> > > > +net_add n1 >>> > > > > > >> > > > +sim_add hv1 >>> > > > > > >> > > > +as hv1 >>> > > > > > >> > > > +check ovs-vsctl add-br br-phys >>> > > > > > >> > > > +ovn_attach n1 br-phys 192.168.0.1 >>> > > > > > >> > > > +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ >>> > > > > > >> > > > + set interface hv1-vif1 >>> external-ids:iface-id=ls1-lp1 >>> > > > > > >> > > > + >>> > > > > > >> > > > +check ovn-nbctl ls-add ls1 >>> > > > > > >> > > > + >>> > > > > > >> > > > +check ovn-nbctl lsp-add ls1 ls1-lp1 \ >>> > > > > > >> > > > +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" >>> > > > > > >> > > > + >>> > > > > > >> > > > +wait_for_ports_up >>> > > > > > >> > > > +ovn-appctl -t ovn-controller vlog/set file:dbg >>> > > > > > >> > > > + >>> > > > > > >> > > > +# Get the OF table numbers >>> > > > > > >> > > > +acl_eval=$(ovn-debug lflow-stage-to-oftable >>> > ls_out_acl_eval) >>> > > > > > >> > > > +acl_action=$(ovn-debug lflow-stage-to-oftable >>> > > > ls_out_acl_action) >>> > > > > > >> > > > + >>> > > > > > >> > > > +dp_key=$(printf "%x" $(fetch_column datapath >>> tunnel_key >>> > > > > > external_ids:name=ls1)) >>> > > > > > >> > > > +port_key=$(printf "%x" $(fetch_column port_binding >>> > tunnel_key >>> > > > > > logical_port=ls1-lp1)) >>> > > > > > >> > > > + >>> > > > > > >> > > > +ovn-nbctl create address_set name=as1 >>> > > > > > >> > > > +check ovn-nbctl acl-add ls1 to-lport 100 'outport == >>> > > > > > "ls1-lp1" && ip4.src == $as1' drop >>> > > > > > >> > > > +check ovn-nbctl add address_set as1 addresses >>> 10.0.0.0/24 >>> > > > > > <http://10.0.0.0/24> >>> > > > > > >> > > > +check ovn-nbctl --wait=hv sync >>> > > > > > >> > > > + >>> > > > > > >> > > > +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int >>> > > > > > table=$acl_eval,reg15=0x$port_key | grep -v reply | awk >>> '{print $7, >>> > $8}' >>> > > > > > | sort], [0], [dnl >>> > > > > > >> > > > +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src= >>> > 10.0.0.0/24 >>> > > > > > <http://10.0.0.0/24> >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > +]) >>> > > > > > >> > > > + >>> > > > > > >> > > > +check ovn-nbctl add address_set as1 addresses >>> 10.0.0.1 >>> > > > > > >> > > > +check ovn-nbctl add address_set as1 addresses >>> 10.0.0.2 >>> > > > > > >> > > > +check ovn-nbctl add address_set as1 addresses >>> 10.0.0.3 >>> > > > > > >> > > > +check ovn-nbctl add address_set as1 addresses >>> 10.0.0.4 >>> > > > > > >> > > > +check ovn-nbctl --wait=hv sync >>> > > > > > >> > > > + >>> > > > > > >> > > > +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int >>> > > > > > table=$acl_eval,reg15=0x$port_key | grep -v reply | awk >>> '{print $7, >>> > $8}' >>> > > > > > | sort], [0], [dnl >>> > > > > > >> > > > +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src= >>> > 10.0.0.0/24 >>> > > > > > <http://10.0.0.0/24> >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > +]) >>> > > > > > >> > > > + >>> > > > > > >> > > > + >>> > > > > > >> > > > +check ovn-appctl inc-engine/recompute >>> > > > > > >> > > > + >>> > > > > > >> > > > +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int >>> > > > > > table=$acl_eval,reg15=0x$port_key | grep -v reply | awk >>> '{print $7, >>> > $8}' >>> > > > > > | sort], [0], [dnl >>> > > > > > >> > > > +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src= >>> > 10.0.0.0/24 >>> > > > > > <http://10.0.0.0/24> >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.2 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.3 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > >>> +priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.4 >>> > > > > > actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,$acl_action) >>> > > > > > >> > > > +]) >>> > > > > > >> > > > + >>> > > > > > >> > > > +OVN_CLEANUP([hv1]) >>> > > > > > >> > > > +AT_CLEANUP >>> > > > > > >> > > > -- >>> > > > > > >> > > > 2.44.0 >>> > > > > > >> > > > >>> > > > > > >> > >>> > > > > > >> > >>> > > > > > >> > Thank you Han, >>> > > > > > >> > >>> > > > > > >> > I would like to start the discussion about the >>> possibility of >>> > > > > > >> > backporting this change. The reason for the backport >>> being >>> > that >>> > > > this >>> > > > > > >> > change might be beneficial to ovn-kubernetes users, as >>> this >>> > was >>> > > > > > >> > actually observed in ovn-kubernetes with network >>> policies. I >>> > know >>> > > > that >>> > > > > > >> > strictly speaking this is not a bug fix, because this >>> > limitation >>> > > > was >>> > > > > > >> > well documented, however I would like to hear the >>> opinion of >>> > > > others. >>> > > > > > >> >>> > > > > > >> >>> > > > > > >> I have no objections if this improvement is strongly >>> required >>> > for >>> > > > > > released versions, even though it is not a bug. May I ask for >>> which >>> > > > > > releases do you want this to be backported? >>> > > > > > > >>> > > > > > > >>> > > > > > > We have a request for 23.09 and 24.03. >>> > > > > > > >>> > > > > > Ok. I will wait for input from other maintainers. It will be a >>> > tradeoff >>> > > > > > between risk and performance gains. Mark, Numan and Dumitru, >>> what >>> > do you >>> > > > > > think? >>> > > > > > >>> > > > > > Thanks, >>> > > > > > Han >>> > > > > >>> > > > > I'm OK with the patch being backported. I think our test >>> coverage is >>> > > > > sufficient to uncover problems that may arise from the backport, >>> so >>> > the >>> > > > > risk should be minimal. >>> > > > > >>> > > > >>> > > > Thanks Numan and Mark for confirming, so we got a quorum. I >>> backported >>> > the >>> > > > patch to 24.03 and 23.09. >>> > > >>> > > This patch is using the "ovn-debug" binary which is missing in >>> > > branch-24.03 and branch-23.09. >>> > > Because of this, the test case "AS I-P and recompute consistency" is >>> > > failing. I backported >>> > > the commit "117573e2d279" to these branches. >>> > > >>> > > While backporting to branch-23.09, had to move the pipeline stage >>> macros >>> > from >>> > > northd.c to northd.h. Running the tests in my github repo before >>> pushing >>> > > to upstream branch-23.09. >>> > > >>> > >>> > Oh, no. Thanks Numan for fixing! I ran test locally after switching >>> > branches and all were successful, likely because the ovn-debug binary >>> is >>> > already in my environment, so I pushed late at night and see this just >>> now. >>> > Sorry for the trouble. >>> > >>> >>> No worries. I passed for me locally too until I deleted the binary. >>> >>> Numan >>> >>> > Regards, >>> > Han >>> > >>> > > Numan >>> > > >>> > > > >>> > > > Han >>> > > > >>> > > > > > >>> > > > > > >> >>> > > > > > >> >>> > > > > > >> Thanks, >>> > > > > > >> Han >>> > > > > > >> >>> > > > > > >> > >>> > > > > > >> > Thanks, >>> > > > > > >> > Ales >>> > > > > > >> > -- >>> > > > > > >> > >>> > > > > > >> > Ales Musil >>> > > > > > >> > >>> > > > > > >> > Senior Software Engineer - OVN Core >>> > > > > > >> > >>> > > > > > >> > Red Hat EMEA >>> > > > > > >> > >>> > > > > > >> > [email protected] <mailto:[email protected]> >>> > > > > > >> > >>> > > > > > > >>> > > > > > > >>> > > > > > > >>> > > > > > > -- >>> > > > > > > >>> > > > > > > Ales Musil >>> > > > > > > >>> > > > > > > Senior Software Engineer - OVN Core >>> > > > > > > >>> > > > > > > Red Hat EMEA >>> > > > > > > >>> > > > > > > [email protected] <mailto:[email protected]> >>> > > > > >>> > > > _______________________________________________ >>> > > > dev mailing list >>> > > > [email protected] >>> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> > _______________________________________________ >>> > dev mailing list >>> > [email protected] >>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > [email protected] > <https://red.ht/sig> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
