On Tue, Jun 23, 2020 at 10:04 PM Mark Michelson <mmich...@redhat.com> wrote:
> On 6/23/20 11:08 AM, Dumitru Ceara wrote: > > On 6/23/20 4:56 PM, Mark Michelson wrote: > >> Acked-by: Mark Michelson <mmich...@redhat.com> > Thanks Dumitru and Mark (for the reviews). I applied this patch to master with the below changes in the tests/ovn.at. I was just playing around how the patch works and thought we could add this to the test. diff --git a/tests/ovn.at b/tests/ovn.at index de62f3883..6587fd8ff 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -112,6 +112,9 @@ a/b => a error("`/' is only valid as part of `//' or `/*'.") b 192.168.0.0/255.0.0.0 => 192.0.0.0/8 192.168.0.0/32 192.168.0.0/255.255.255.255 => 192.168.0.0/32 +192.168.0.2/32 +192.168.0.2/30 => 192.168.0.0/30 +192.168.0.2/24 => 192.168.0.0/24 1.2.3.4:5 => 1.2.3.4 : 5 :: Thanks Numan >> > > > > Thanks Mark for the review. > > > >> I was going to suggest printing an informational message if the value > >> gets truncated. However, given the way the lexer works, it's not as > >> simple as just printing a message when you encounter the situation. If > >> we were to add informational messages to the lexer, that should be a > >> separate (and very low-priority) patch. > > > > I was looking into that too but couldn't find a not-so-intrusive and > > clean way to do it. Do you have any suggestions? I can try to send a > > follow up patch if we think it's worth it. > > I gave it about 30 seconds of thought and couldn't think of anything > non-intrusive either. I also have to admit I don't have a ton of > experience in the lexer code, so it would take some research to figure > out a good way to change this. > > I don't think it's worth following up immediately with a separate patch. > I think this gets filed into our "it would be nice to have" pile. > > > > > Thanks, > > Dumitru > > > >> > >> On 6/23/20 4:17 AM, Dumitru Ceara wrote: > >>> It's quite restrictive to not accept ACLs/policies that match on a CIDR > >>> that has non-zero host bits. Right now this generates a lexer error > that > >>> can only be detected in the logs. > >>> > >>> There's no real harm in automatically zero-ing the unmasked bits. > >>> > >>> Reported-at: https://bugzilla.redhat.com/1812820 > >>> Reported-by: Ying Xu <yi...@redhat.com> > >>> Signed-off-by: Dumitru Ceara <dce...@redhat.com> > >>> --- > >>> lib/lex.c | 10 ++-------- > >>> tests/ovn.at | 8 ++++---- > >>> 2 files changed, 6 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/lib/lex.c b/lib/lex.c > >>> index 94f6c77..4d92199 100644 > >>> --- a/lib/lex.c > >>> +++ b/lib/lex.c > >>> @@ -485,16 +485,10 @@ lex_parse_mask(const char *p, struct lex_token > >>> *token) > >>> return p; > >>> } > >>> - /* Check invariant that a 1-bit in the value corresponds to a > >>> 1-bit in the > >>> + /* Apply invariant that a 1-bit in the value corresponds to a > >>> 1-bit in the > >>> * mask. */ > >>> for (int i = 0; i < ARRAY_SIZE(token->mask.be32); i++) { > >>> - ovs_be32 v = token->value.be32[i]; > >>> - ovs_be32 m = token->mask.be32[i]; > >>> - > >>> - if (v & ~m) { > >>> - lex_error(token, "Value contains unmasked 1-bits."); > >>> - break; > >>> - } > >>> + token->value.be32[i] &= token->mask.be32[i]; > >>> } > >>> /* Done! */ > >>> diff --git a/tests/ovn.at b/tests/ovn.at > >>> index 1ff7952..0c0daed 100644 > >>> --- a/tests/ovn.at > >>> +++ b/tests/ovn.at > >>> @@ -79,7 +79,7 @@ a/b => a error("`/' is only valid as part of `//' or > >>> `/*'.") b > >>> 0/0 > >>> 0/1 > >>> -1/0 => error("Value contains unmasked 1-bits.") > >>> +1/0 => 0/0 > >>> 1/1 > >>> 128/384 > >>> 1/3 > >>> @@ -99,7 +99,7 @@ a/b => a error("`/' is only valid as part of `//' or > >>> `/*'.") b > >>> 0X => error("Hex digits expected following 0X.") > >>> 0x0/0x0 => 0/0 > >>> 0x0/0x1 => 0/0x1 > >>> -0x1/0x0 => error("Value contains unmasked 1-bits.") > >>> +0x1/0x0 => 0/0 > >>> 0xffff/0x1ffff > >>> 0x. => error("Invalid syntax in hexadecimal constant.") > >>> @@ -109,7 +109,7 @@ a/b => a error("`/' is only valid as part of > >>> `//' or `/*'.") b > >>> 192.168.0.0/255.255.0.0 => 192.168.0.0/16 > >>> 192.168.0.0/255.255.255.0 => 192.168.0.0/24 > >>> 192.168.0.0/255.255.0.255 > >>> -192.168.0.0/255.0.0.0 => error("Value contains unmasked 1-bits.") > >>> +192.168.0.0/255.0.0.0 => 192.0.0.0/8 > >>> 192.168.0.0/32 > >>> 192.168.0.0/255.255.255.255 => 192.168.0.0/32 > >>> 1.2.3.4:5 => 1.2.3.4 : 5 > >>> @@ -135,7 +135,7 @@ FE:DC:ba:98:76:54 => fe:dc:ba:98:76:54 > >>> 01:00:00:00:00:00/01:00:00:00:00:00 > >>> ff:ff:ff:ff:ff:ff/ff:ff:ff:ff:ff:ff > >>> fe:ff:ff:ff:ff:ff/ff:ff:ff:ff:ff:ff > >>> -ff:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff => error("Value contains unmasked > >>> 1-bits.") > >>> +ff:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff => > >>> fe:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff > >>> fe:x => error("Invalid numeric constant.") > >>> 00:01:02:03:04:x => error("Invalid numeric constant.") > >>> > >> > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev