On 01/04/2021 13:28, gmingchen(陈供明) wrote:
> Correct some words
>
>
> On 2021/4/1, 8:17 PM, "gmingchen(陈供明)" <[email protected]> wrote:
>
>
>
> On 2021/3/30, 4:59 PM, "Mark Gray" <[email protected]> wrote:
>
> On 29/03/2021 13:30, gmingchen(陈供明) wrote:
> >
> > On 2021/3/25, 11:30 PM, "Mark Gray" <[email protected]> wrote:
> >
> > On 19/03/2021 13:01, Dumitru Ceara wrote:
> > > On 3/10/21 2:29 PM, gmingchen(陈供明) wrote:
> > >> From: Gongming Chen <[email protected]>
> > >>
> >
> > Thanks for the patch. Looks like a lot of thought went into the
> > algorithm and it has been interesting to review.
> >
> > Do you know if there are any well-known algorithms to do this?
> It seems
> > like a common problem? If there was, it may be better to use it
> as we
> > could reference standard documentation.
> >
> > Hi Mark,
> > First of all, thanks for the review.
> >
> > Unfortunately, I did not find a well-known algorithms.
>
> Ok. Thats a shame. It's kind of like IP subnetting so I thought there
> may be something.
>
> >
> > >> This patch merges ipv4 addresses with wildcard masks, and
> replaces this
> > >> ipv4 addresses with the combined ip/mask. This will greatly
> reduce the
> > >> entries in the ovs security group flow table, especially
> when the host
> > >> size is large.
> > >>
> > >> Analysis in the simplest scenario, a network 1.1.1.0/24
> network,create 253
> > >> ports(1.1.1.2-1.1.1.254).
> > >> Only focus on the number of ip addresses, the original 253
> addresses will
> > >> be combined into 13 addresses.
> > >> 1.1.1.2/31
> > >> 1.1.1.4/30
> > >> 1.1.1.8/29
> > >> 1.1.1.16/28
> > >> 1.1.1.32/27
> > >> 1.1.1.64/26
> > >> 1.1.1.128/26
> > >> 1.1.1.192/27
> > >> 1.1.1.224/28
> > >> 1.1.1.240/29
> > >> 1.1.1.248/30
> > >> 1.1.1.252/31
> > >> 1.1.1.254
> > >>
> > >> Some scenes are similar to the following:
> > >> 1.1.1.2, 1.1.1.6
> > >> After the combine:
> > >> 1.1.1.2/255.255.255.251
> > >> You can use ovn-match-ip utility to match ip.
> > >> such as:
> > >> ovs-ofctl dump-flows br-int | ovn-match-ip 1.1.1.6
> > >> 1.1.1.2/255.255.255.251 will show.
> > >>
> > >> Simple description of the algorithm.
> > >> There are two situations
> > >> 1. Combine once
> > >> such as:
> > >> 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
> > >> Combined into: 1.1.1.0/31, 1.0.1.0/31
> > >> 2. Combine multiple times
> > >> 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
> > >> Combined into: 1.0.1.0/255.254.255.254
> > >>
> > >> Considering the actual scene and simplicity, the first case
> is used to
> > >> combine once.
> > >>
> > >> ...00...
> > >> ...01...
> > >> ...10...
> > >> ...11...
> > >> "..." means the same value omitted.
> > >> Obviously, the above value can be expressed as
> ...00.../11100111. This
> > >> continuous interval that can be represented by one or
> several wildcard
> > >> masks is called a segment.
> > >> Only if all 2<<n values from the minimum value 00...(n) to
> 11...(n)
> > >> exist, can they be combined into 00...(n)/00...( n)
> > >>
> > >> First sort all the values by size. Iterate through each
> value.
> > >> 1. Find a new segment, where two values differ only by 1
> bit, such as
> > >> ...0... and ...1...
> > >> diff = ip_next ^ ip
> > >> if (diff & (diff-1)) == 0
> > >> new_segment = true
> > >> The first non-zero place in the high direction of ip is the
> end of the
> > >> segment(segment_end).
> > >> For example...100... and...101..., the segment_end is
> ...111...
> > >>
> > >> 2. Count the number of consecutive and less than
> continuous_size in the
> > >> segment.
> > >> diff = ip_next - ip
> > >> if (diff & (diff-1)) == 0 && ip_next <= segment_end
> > >> continuous_size++
> > >>
> > >> 3. Combine different ip intervals in the segment according to
> > >> continuous_size.
> > >> In continuous_size, from the highest bit of 1 to the lowest
> bit of 1, in
> > >> the order of segment start, each bit that is 1 is a
> different ip interval
> > >> that can be combined with a wildcard mask.
> > >> For example, 000, 001, 010:
> > >> continuous_size: 3 (binary 11), segment_start: 000
> > >> mask: ~(1 << 1 - 1) = 110; ~(1 << 0 - 1) = 111;
> > >> Combined to: 000/110, 010/111
> > >>
> > >> 4. The ip that cannot be recorded in a segment will not be
> combined.
> > >>
> > >> Signed-off-by: Gongming Chen <[email protected]>
> > >> ---
> > >
> > > Hi Gongming,
> > >
> > > Sorry for the delayed review.
> > >
> > > I have a few general remarks/concerns and some specific
> comments inline.
> > > First, the general remarks.
> > >
> > > I'm wondering if it would make more sense for this wildcard
> combination
> > > of IPs to be done outside OVN, in the CMS, for the following
> reasons:
> > >
> > > - it's IPv4 specific.
> > > - the CMS usually has more information about when it is
> matching on IP
> > > sets and can probably optimize the match it uses in the ACL.
> >
> >
> > > - the code in expr_const_sets_add() used to be a relatively
> direct
> > > translation from sets of constants to sets of port names/IPs;
> this
> > > changes with the optimization your proposing.
> > > - the new utility, ovn-match-ip, would be useful but I'm
> worried that
> > > users will often forget to use it.
> > I'd like to understand more about the benefits of this change,
> what
> > problem is it solving? I agree with Dumitru that it will make
> debugging
> > quite a bit more difficult (even with the new tool) so it would
> be good
> > to understand the tradeoff?
> >
> > The benefit is more due to the reduction of the flow table.
> > 1. Fewer flow tables will install to ovs faster.
>
> Do you mean flows rather than flow tables?
>
> Yes. Sorry, my bad.
>
> > 2. Before optimization, enabling the openstack remote group feature
> in a
> > large-scale environment may reach the limit of 20,000 flow tables.
> In the
> > application of flows offload, this situation is more likely to
> happen.
> > 3. Fewer flow tables usually means faster flow table lookups and
> stability.
> > 4. When remote group is enabled, acl(security group) is usually a
> challenge,
> > and this challenge usually comes from a huge number of ip matching
> entries.
> > And this will reduce the challenge.
> >
>
> Ok I understand that the main benefit is performance. Do you have any
> bench-marking figures? I am not trying to be obstructive, but this
> will
> change the user interface so I think it would be good to understand
> the
> benefit. In the end, it will be the maintainers who will need to
> decide
> if the benefit outweighs that change and should be applied. I do think
> it will make things more difficult to debug problems in the flow table
>
> Yes, it is indeed harder to debug. The first version of patch included the
> function of turning this feature on or off. In this way, users can turn on
> this feature in a large-scale environment. By the way, after having this
> ovn-match-ip tool, judging from the use in our own environment, it has
> little effect on debugging. The function that can be turned on or off,
> will it be more flexible?
>
> I have the approximate data on the reduction in the number of flows.
> Of course, more benefits are based on the reduction of the flows.
> In our own environment, in most cases, the flows will be reduced by more
> than half. The larger the scale, the denser the IP address, the better
> the effect of the flows reduction .
I had a conversation with Dumitru and this patch came up in the
conversation. He made an interesting suggestion (Dumitru, please correct
me if I get this wrong) that this could be refactored as an external
tool. This cmdline tool could, for example, take a set of IP addresses
and return the wildcard representation. For example,
# ./ovs-new-tool <set of addresses>
<ip address/prefix> ... <ip address/prefix>
Then this tool could optionally be used by a CMS. The other advantage of
this is that it could also be used in other places.
>
> > >
> > > I'm curious about your and the maintainers' opinion on these
> points.
> > >
> > > Another general remark is to increment the patch version when
> submitting
> > > a new revision. From what I can see this specific patch is
> at v7. When
> > > sending a v8 you could add "-v8" when generating the patch,
> e.g.:
> > >
> > > git format-patch --subject-prefix="PATCH ovn" -v8 -M
> origin/master
> > >
> > > Would generate a patch with subject:
> > >
> > > "Subject: [PATCH ovn v8] ...."
> > >
> > > This usually helps when reviewing patches that go through
> multiple
> > > iterations.
> > >
> > > More specific comments inline.
> > >
> > >> debian/ovn-common.install | 1 +
> > >> lib/expr.c | 219
> ++++++++++++++++++++++++++++++++++++++
> > >> rhel/ovn-fedora.spec.in | 1 +
> > >> tests/ovn.at | 136 +++++++++++------------
> > >> tests/test-ovn.c | 9 ++
> > >> utilities/automake.mk | 5 +-
> > >> utilities/ovn-match-ip.in | 78 ++++++++++++++
> > >> 7 files changed, 382 insertions(+), 67 deletions(-)
> > >> create mode 100755 utilities/ovn-match-ip.in
> > >>
> > >> diff --git a/debian/ovn-common.install
> b/debian/ovn-common.install
> > >> index 8e5915724..0095df918 100644
> > >> --- a/debian/ovn-common.install
> > >> +++ b/debian/ovn-common.install
> > >> @@ -5,6 +5,7 @@ usr/bin/ovn-ic-nbctl
> > >> usr/bin/ovn-ic-sbctl
> > >> usr/bin/ovn-trace
> > >> usr/bin/ovn-detrace
> > >> +usr/bin/ovn-match-ip
> > >> usr/share/ovn/scripts/ovn-ctl
> > >> usr/share/ovn/scripts/ovndb-servers.ocf
> > >> usr/share/ovn/scripts/ovn-lib
> > >> diff --git a/lib/expr.c b/lib/expr.c
> > >> index f061a8fbe..0a7203817 100644
> > >> --- a/lib/expr.c
> > >> +++ b/lib/expr.c
> > >> @@ -1060,6 +1060,200 @@ expr_constant_set_destroy(struct
> expr_constant_set *cs)
> > >> }
> > >> }
> > >>
> > >> +struct ip_in
> > >> +{
> > >> + uint32_t *ip;
> > >> + size_t size;
> > >> +};
> > >
> > > I don't think we really need this, we could just pass the
> IPv4 address
> > > array to combine_ipv4_in_mask(), along with the number of
> items in the
> > > array.
> > >
> > >> +
> > >> +struct ip_out_entry
> > >> +{
> > >> + uint32_t ip;
> > >> + uint32_t mask;
> > >> + bool masked;
> > >> +};
> > >> +
> > >> +struct ip_out
> > >> +{
> > >> + struct ip_out_entry *entries;
> > >> + size_t used;
> > >> + size_t size;
> > >> +};
> > >
> > > AFAIU, we can never have more "out" entries than "in"
> entries. This
> > > means we could be conservative and always allocate as many
> output
> > > entries as ip_in entries and pass the preallocated struct
> ip_out_entry
> > > array to combine_ipv4_in_mask().
> > >
> > > This would also remove the need to resize the "out" array.
> > >
> > >> +
> > >> +static int
> > >> +compare_mask_ip(const void *a, const void *b)
> > >
> > > The implementation doesn't really use the fact that 'a' and
> 'b' are IPs.
> > > They're just compared as integers. We might want to use a
> different
> > > name and move this function to a common module as it could
> potentially
> > > be used in multiple places in the future.
> > >
> > >> +{
> > >> + uint32_t a_ = *(uint32_t *)a;
> > >> + uint32_t b_ = *(uint32_t *)b;
> > >> +
> > >> + return a_ < b_ ? -1 : a_ > b_;
> > >> +}
> > >> +
> > >> +/* Function to check ip return data and xrealloc. */
> > >> +static void
> > >> +check_realloc_ip_out(struct ip_out *ip_out_data){
> > >> + if (ip_out_data->used >= ip_out_data->size) {
> > >> + ip_out_data->entries =
> x2nrealloc(ip_out_data->entries,
> > >> + &ip_out_data->size,
> > >> + sizeof
> *ip_out_data->entries);
> > >> + }
> > >> +}
> > >
> > > As mentioned in the comment earlier, we can probably avoid
> using this
> > > function.
> > >
> > >> +
> > >> +/* Simple description of the algorithm.
> > >> + * There are two situations
> > >> + * 1. Combine once
> > >> + * such as:
> > >> + * 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
> > >> + * Combined into: 1.1.1.0/31, 1.0.1.0/31
> > >> + * 2. Combine multiple times
> > >> + * 1.1.1.0 1.1.1.1 1.0.1.0 1.0.1.1
> > >> + * Combined into: 1.0.1.0/255.254.255.254
> > >> + *
> > >> + * Considering the actual scene and simplicity, the first
> case is used to
> > >> + * combine once.
> > >> + *
> > >> + * ...00...
> > >> + * ...01...
> > >> + * ...10...
> > >> + * ...11...
> > >> + * "..." means the same value omitted.
> > >> + * Obviously, the above value can be expressed as
> ...00.../11100111. This
> > >> + * continuous interval that can be represented by one or
> several wildcard
> > >> + * masks is called a segment.
> > >> + * Only if all 2<<n values from the minimum value 00...(n)
> to 11...(n)
> > >> + * exist, can they be combined into 00...(n)/00...( n)
> > >> + *
> > >> + * First sort all the values by size. Iterate through each
> value.
> > >> + * 1. Find a new segment, where two values differ only by 1
> bit, such as
> > >> + * ...0... and ...1...
> > >> + * diff = ip_next ^ ip
> > >> + * if (diff & (diff-1)) == 0
> > >> + * new_segment = true
> > >> + * The first non-zero place in the high direction of ip is
> the end of the
> > >> + * segment(segment_end).
> > >> + * For example...100... and...101..., the segment_end is
> ...111...
> > >> + *
> > >> + * 2. Count the number of consecutive and less than
> continuous_size in the
> > >> + * segment.
> > >> + * diff = ip_next - ip
> > >> + * if (diff & (diff-1)) == 0 && ip_next <= segment_end
> > >> + * continuous_size++
> > >> + *
> > >> + * 3. Combine different ip intervals in the segment
> according to
> > >> + * continuous_size.
> > >> + * In continuous_size, from the highest bit of 1 to the
> lowest bit of 1, in
> > >> + * the order of segment start, each bit that is 1 is a
> different ip interval
> > >> + * that can be combined with a wildcard mask.
> > >> + * For example, 000, 001, 010:
> > >> + * continuous_size: 3 (binary 11), segment_start: 000
> > >> + * mask: ~(1 << 1 - 1) = 110; ~(1 << 0 - 1) = 111;
> > >> + * Combined to: 000/110, 010/111
> > >> + *
> > >> + * 4. The ip that cannot be recorded in a segment will not
> be combined. */
> > >
> > > Thanks for adding the detailed description here. However,
> it's a bit
> > > confusing because the names used in the description don't
> match the
> > > variable names used in the implementation below. Maybe it's
> better to
> > > not use names in the description?
> >
> > I would prefer that with some refactoring of the code below and
> some
> > in-line comments, we could make the documentation unnecessary
> and remove
> > or reduce it.
> >
> > Yes, less in-line comments will be better.
> >
> > >
> > >> +static void
> > >> +combine_ipv4_in_mask(struct ip_in *ip_in_data, struct
> ip_out *ip_out_data){
> > >
> > > Please move the curly brace on a new line.
> > >
> > >> + bool recording = false;
> > >> + uint32_t i, diff, connect, start, end, continuous_size,
> mask_base;
> > >
> > > Some of these can be moved in the specific blocks where
> they're used.
> > >
> > >> +
> > >> + start = 0;
> > >> + continuous_size = 0;
> > >> + mask_base = 0;
> > >> + end = 0;
> > >> +
> > >> + qsort(ip_in_data->ip, ip_in_data->size,
> sizeof(uint32_t),
> > >
> > > Please use "sizeof *ip_in_data->ip".
> > >
> > >> + compare_mask_ip);
> > >> + memset(ip_out_data, 0, sizeof(struct ip_out));
> > >> +
> > >> + for (i = 0; i < ip_in_data->size; i++) {
> > >> + if (i + 1 >= ip_in_data->size) {
> > >> + if (!recording) {
> > >> + goto end_when_not_recording;
> > >> + } else {
> > >> + goto end_when_recording;
> > >> + }
> >
> > At a first glance, I don't know what "recording" is. Maybe it
> needs
> > another name ("combining" or even "new_segment" as you have in
> the
> > documentation?) or some comments.
> >
> > Yes, the combining name will be more readable.
> >
> > >
> > > IMO it makes the code very hard to read if we have gotos
> jumping inside
> > > if/else branches or inside loops. This is also usually a
> sign that
> > > we're trying to avoid code duplication. A potentially
> simpler and more
> > > readable way to do that is to add functions for the specific
> operations
> > > we don't want to duplicate.
> > >
> > >> + }
> > >> + /* Not recording in a segment.
> > >> + * Record a new segment or not combine. */
> > >> + if (!recording) {
> > >> + connect = ip_in_data->ip[i + 1] ^
> ip_in_data->ip[i];
> >
> > Maybe this ^ could be a macro as it would help explain why you
> are
> > XORing these two IP addresses.
> >
> > >> + /* Only one bit different. */
> > >> + if ((connect & (connect - 1)) == 0) {
> >
> > Maybe you could use 'count_1bits()' as it would make it clearer
> >
> > How about something like:
> >
> > #define COUNT_BITS_DIFFERENT (uint32_t a, uint32_t b)
> count_1bits((a) ^ (b))
> >
> > and
> > if (COUNT_BITS_DIFFERENT(...) == 1)
> >
> > What do you think?
> >
> > static inline bool
> > is_1bit_different(uint32_t a, uint32_t b)
> > {
> > return (a ^ b) & (a ^ b - 1) == 0;
> > }
> >
> > Or add is_1bit_different to ovs/util.h and use is_1bit_different()
> which
> > one do you think is better?
>
> That would be good as well. The main reason is to make the code easier
> to understand. Its a complex algorithm and I found it difficult to
> follow so I think it should be refactored until it
> is easier to understand so this patch may need to go through a few
> iterations. If it is easier to understand, it will be easier to
> maintain.
>
> Ok, I will make the code more clear and easier to read.
>
> >
> > >> + recording = true;
> > >> + start = ip_in_data->ip[i];
> > >> + continuous_size = 2;
> > >> + mask_base = connect;
> > >> +
> > >> + int j = 0;
> > >> + end = start;
> >
> > Maybe the name could be changed to 'max' or something like that
> as it is
> > not the actual end but the maximum possible ip in the segment?
> >
> > Yes, max will be better.
> >
> > >> + /* The first non-zero place in the high
> direction is
> > >> + * the end of the segment. *
> > Maybe bitwise_scan() could be used to scan for the bit that it
> is set?
> >
> > When using bitwise_scan() , it might be like:
> >
> > int ofs_base, ofs_max
> >
> > ofs_base = bitwise_scan(&start, 32, true, 0, 32);
> So this will find the position of the first 'set' bit?
> > if(ofs_base >= 31){
> > max = 1 << 31 | start;
> > } else {
> > ofs_max = bitwise_scan(&start, 32, true, ofs_base + 1, 32);
> This will find the position of the the second 'set' bit?
>
> This means we are looking for the largest pattern like this?
> 11
> 101
> 1001
> 10001
> ..
> ..
>
> Yes, because each continuous segment must start at...000..., we will find
> the
This is a good clarification to put in the definition of a segment (i.e.
starts at 0..0). So you are looking for the longest string of zeros in a
segment.
> longest and highest 0. For example, 1000, 1001, starting from 1000, the
> largest segment that can be merged starting from 1000, but cannot exceed
> 1111 (that is, max), because the high 1 in 1000 is not 0. Similarly,
> 101000, 101001..., the segment starting from 10100, the maximum cannot
> exceed 10111 (that is, max)
>
> 101000/111000
> contain
>
> 101000
> ...
> 101111
>
> 101000 is the beginning of this 1000/1000 segment.
> 101111 is the maximum value of this segment.
It is a difficult algorithm to grasp (and hence document) but I think I
get it now.
>
> > max = start | ((1 << (ofs_max - ofs_base) - 1) << ofs_base)
> And this will build a mask between those two bit positions and then
> apply to 'start'?
>
> Yes, this is to set the 1 corresponding to the mask to start, that is, the
> longest ip 0 at the beginning of this segment is set to 1, which becomes
> the largest ip value of this segment.
>
> Yes, because each continuous segment must start at...000..., we will find the
> longest and highest 0. For example, 1000, 1001, starting from 1000, the
> largest segment that can be merged starting from 1000, but cannot exceed
> 1111 (that is, max), because the high 1 in 1000 is not 0. Similarly,
> 101000, 101001..., the segment starting from 101000, the maximum cannot
> exceed 101111 (that is, max)
>
> 101000/111000
> contain
>
> 101000
> ...
> 101111
>
> 101000 is the beginning of this 101000/101000 segment.
> 101111 is the maximum value of this segment.
>
> > max = start | ((1 << (ofs_max - ofs_base) - 1) << ofs_base)
> And this will build a mask between those two bit positions and then
> apply to 'start'?
>
> Yes, this is to set the 1 corresponding to the mask to start, that is, the
> longest ip 0 at the beginning of this segment is set to 1, which becomes
> the largest ip value of this segment.
Does the largest ip value not have all ones?
>
> > }
> >
> > Is this more complicated, do you think?
> IMO, seemed a bit easier to understand.
>
> This is really easier to understand. But the code structure is a bit more
> cumbersome. How about adding this example
> '''
> 101000/111000
> contain
>
> 101000
> ...
> 101111
>
> 101000 is the beginning of this 1000/1000 segment.
>
> correct--> 101000 is the beginning of this 101000/101000 segment.
>
> 101111 is the maximum value of this segment.
> '''
> to the comment of this while code? Will it be easier to read? Because I
> think the while code will be more efficient.
Personally, I am not really concerned about the efficiency of the code
as it is on the control plane. I would rather the code was simple to
understand quickly. I want to make sure that if I (or someone else) was
looking at this code in a year or two from now they could easily see
what was going on.
>
> >
> > >> + while (j < 32 && ((start & (mask_base <<
> j)) == 0)) {
> > >> + end |= (mask_base << j);
> > >> + j++;
> > >> + }
> >
> > I think this could use a comment to explain why this is the
> case. I kind
> > of get it but it took me a while and I am still not 100% sure.
> As I
> > mentioned elsewhere, I think this is really looking for the
> maximum
> > potential IP address rather than what will actually be the the
> end value.
> >
> > Yes, this is looking for the maximum potential IP address.
> > I will add a comment like:
> > This is looking for the maximum potential IP address. Used to
> terminate
> > the combining of a segment
>
> Maybe adding more formalized language describing 'segments' and then
> referring to this consistently in the code and comments would help.
> For
> example, i think this description is true:
>
> --
> A 'segment' is described by:
> 1) A 'segment_mask' which is a bit mask of length 'segment_mask_len'
> bits that begins at a bit offset of 'segment_mask_offset'.
> 2) A 'segment_id' which uniquely identifies a 'segment' for a given
> 'segment_mask'. An IP address is a member of a 'segment' if it belongs
> to that 'segment_id'. The 'segment_id' is obtained by a bitwise AND of
> the inverse of the 'segment_mask' with that IP address. This is
> similar
> to the Network ID of an IPv4 address.
>
> A set of 'segment's can be combined if all IP addresses in the segment
> are present in the set.
> --
>
> To describe the above code snippet, you could state something like:
>
> --
> 'segment_max' is the numerically largest IP address in a 'segment'. It
> can be found by a bitwise OR of the 'segment_mask' with the
> 'segment_id'. In order to find the largest possible 'segment', we
> attempt to find the widest 'segment_mask' by ...
> ...
> ..
>
>
> What do you think?
>
> Amazing! I will add the description of segments to the comments, thanks!
>
> >
> > >> +
> > >> + continue;
> > >> + /* Different segments and different bit, dnot
> combine. */
> > >> + } else {
> > >> +end_when_not_recording:
> > >
> > > Same here.
> > >
> > >> + check_realloc_ip_out(ip_out_data);
> > >> +
> > >> + ip_out_data->entries[ip_out_data->used].ip =
> > >> + ip_in_data->ip[i];
> > >> +
> ip_out_data->entries[ip_out_data->used].masked = false;
> > >> + ip_out_data->used++;
> > >> +
> > >> + continue;
> > >> + }
> > >> + /* Recording in the current segment. */
> > >> + } else {
> > >> + diff = ip_in_data->ip[i + 1] -
> ip_in_data->ip[i];
> > >> + /* Ignore equal node. */
> > >> + if (diff == 0) {
> > >> + continue;
> > >> + }
> >
> > Why would they be equal? And if they were, should we not check
> this in
> > the non-recording case above as well?
> >
> > Under normal circumstances, it will not be equal, here is to deal
> with
> > incorrect equality.
> Maybe add a comment to explain.
> Ok.
> > In non-recording case, it also needs to be checked, my bad.
> >
> > >> + /* Stop recording and combine, or continue
> recording. */
> > >> + if (((diff & (diff - 1)) != 0)
> > >> + || (ip_in_data->ip[i + 1] > end)) {
> > >> +end_when_recording:
> > >
> > > Here too.
> > >
> > >> + recording = false;
> > >> + while (continuous_size) {
> > >> + check_realloc_ip_out(ip_out_data);
> > >> +
> > >> + int segment_power, pow_base;
> > >> + if (continuous_size == 0) {
> > >> + segment_power = 0;
> > >> + } else {
> > >> + segment_power = 31 -
> clz32(continuous_size);
> > >> + }
> > >> +
> > >> + if (mask_base == 0) {
> > >> + pow_base = 0;
> > >> + } else {
> > >> + pow_base = 31 - clz32(mask_base);
> > >> + }
> > >> +
> > >> +
> ip_out_data->entries[ip_out_data->used].mask =
> > >> + ~(((1 << segment_power) - 1) <<
> pow_base);
> >
> > Maybe bitwise_one() could be used to build the mask.
> >
> > As a general comment, it can take some time to reason what bit
> > manipulation code is actually doing. I think it would be good
> to use
> > some of the OVS/OVN bit manipulation helper functions anywhere
> that is
> > appropriate in this code. This will improve the readability.
> >
> > Yes, bitwise_one() is better.
> > Thanks, I try to use OVS/OVN bit manipulation helper functions.
> >
> > >> +
> ip_out_data->entries[ip_out_data->used].ip =
> > >> +
> ip_out_data->entries[ip_out_data->used].mask
> > >> + & start;
> > >> +
> ip_out_data->entries[ip_out_data->used].masked = true;
> > >> + ip_out_data->used++;
> > >> +
> > >> + continuous_size &= (~(1 <<
> segment_power));
> > >> + start =
> ip_out_data->entries[ip_out_data->used - 1].ip
> > >> + + (1 << (segment_power +
> pow_base));
> >
> > Could you move 'ip_out_data->used++' to here and change the
> index above
> > ^ to 'ip_out_data->used'?
> >
> > Ok, thanks.
> >
> > >> + }
> > >> + } else {
> > >> + continuous_size++;
> >
> > Maybe the code in the 'if' statement could be simplified by
> calculating
> > the 'pow_base' and 'segment_power' here at each iteration? I'm
> not sure
> > but I suspect it might at least remove the
> 'while(continuous_size)' loop?
> >
> > Sorry, I don’t know if I understand you correctly. As dumit
> suggested, the
> > while statement will be implemented with a function to eliminate
> the goto
> > statement.
>
> Apologies for my vague comment, let's see how Dumitru's suggestion
> looks
> and we can work from there.
>
> > I am not sure how to delete the "while(continuous_size)" loop.
> >
> > But I noticed that
> > if (continuous_size == 0) {
> > segment_power = 0;
> > } else {
> > segment_power = 31 - clz32(continuous_size);
> > }
> > can be replaced by
> > segment_power = 31 - clz32(continuous_size);
> >
> > And
> > if (mask_base == 0) {
> > pow_base = 0;
> > } else {
> > pow_base = 31 - clz32(mask_base);
> > }
> > can be moved outside of while.
> >
> > do you mean this?
>
> I didnt but I think that is good, lets try to minimize the complexity
> by
> removing as many loops, jumps and branches as we can
>
> Ok.
>
> >
> > >> + }
> > >> +
> > >> + continue;
> > >> + }
> > >> + }
> > >> +}
> > >> +
> > >> /* Adds an constant set named 'name' to 'const_sets',
> replacing any existing
> > >> * constant set entry with the given name. */
> > >> void
> > >> @@ -1074,6 +1268,11 @@ expr_const_sets_add(struct shash
> *const_sets, const char *name,
> > >> cs->in_curlies = true;
> > >> cs->n_values = 0;
> > >> cs->values = xmalloc(n_values * sizeof *cs->values);
> > >> + struct ip_out ip_out_data;
> > >> + struct ip_in ip_in_data;
> > >> + ip_in_data.ip = xmalloc(n_values * sizeof(uint32_t));
> > >> + ip_in_data.size = 0;
> > >> +
> > >> if (convert_to_integer) {
> > >> cs->type = EXPR_C_INTEGER;
> > >> for (size_t i = 0; i < n_values; i++) {
> > >> @@ -1086,6 +1285,9 @@ expr_const_sets_add(struct shash
> *const_sets, const char *name,
> > >> && lex.token.type != LEX_T_MASKED_INTEGER) {
> > >> VLOG_WARN("Invalid constant set entry:
> '%s', token type: %d",
> > >> values[i], lex.token.type);
> > >> + } else if (lex.token.type == LEX_T_INTEGER
> > >> + && lex.token.format == LEX_F_IPV4) {
> > >> + ip_in_data.ip[ip_in_data.size++] =
> ntohl(lex.token.value.ipv4);
> > >> } else {
> >
> > Seems like it would also need an IPv6 version.
> >
> > Thinking about this a bit more, why does this need to be
> specific to
> > IPv4 - surely there is a similar problem across all masked
> integers?
> >
> > Considering that ipv6 does not have a concept like ipv4 mask, and
> private
> > networks use ipv6 is relatively small, so I did not consider ipv6.
> > Of course, this algorithm is also suitable for ipv6 in addition to
> the
> > uint32_t type. My idea is to add ipv6 part of the support when
> encountering
> > this kind of scene in the ipv6 scene. what do you think?
>
> I guess its up to the maintainers.
>
> If ipv6 is necessary, I can expand the type size to support ipv6.
>
> Thanks so much,
> Gongming
>
> >
> > Thanks,
> > Gongming
> >
> > >> union expr_constant *c =
> &cs->values[cs->n_values++];
> > >> c->value = lex.token.value;
> > >> @@ -1105,6 +1307,23 @@ expr_const_sets_add(struct shash
> *const_sets, const char *name,
> > >> }
> > >> }
> > >>
> > >> + if (ip_in_data.size > 0) {
> > >> + combine_ipv4_in_mask(&ip_in_data, &ip_out_data);
> > >> + for (size_t i = 0; i < ip_out_data.used; ++i) {
> > >> + union expr_constant *c =
> &cs->values[cs->n_values++];
> > >> + memset(&c->value, 0, sizeof c->value);
> > >> + memset(&c->mask, 0, sizeof c->mask);
> > >> + c->value.ipv4 =
> htonl(ip_out_data.entries[i].ip);
> > >> + c->format = LEX_F_IPV4;
> > >> + c->masked = ip_out_data.entries[i].masked;
> > >> + if (c->masked) {
> > >> + c->mask.ipv4 =
> htonl(ip_out_data.entries[i].mask);
> > >> + }
> > >> + }
> > >> + free(ip_out_data.entries);
> > >> + }
> > >> +
> > >> + free(ip_in_data.ip);
> > >> shash_add(const_sets, name, cs);
> > >> }
> > >>
> > >> diff --git a/rhel/ovn-fedora.spec.in
> b/rhel/ovn-fedora.spec.in
> > >> index 6716dd0d2..1a0abff06 100644
> > >> --- a/rhel/ovn-fedora.spec.in
> > >> +++ b/rhel/ovn-fedora.spec.in
> > >> @@ -455,6 +455,7 @@ fi
> > >> %{_bindir}/ovn-sbctl
> > >> %{_bindir}/ovn-trace
> > >> %{_bindir}/ovn-detrace
> > >> +%{_bindir}/ovn-match-ip
> > >> %{_bindir}/ovn-appctl
> > >> %{_bindir}/ovn-ic-nbctl
> > >> %{_bindir}/ovn-ic-sbctl
> > >> diff --git a/tests/ovn.at b/tests/ovn.at
> > >> index b751d6db2..a243cd6f4 100644
> > >> --- a/tests/ovn.at
> > >> +++ b/tests/ovn.at
> > >> @@ -618,20 +618,32 @@ ip,nw_src=10.0.0.3
> > >> ])
> > >> AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl
> > >> ip,nw_src=10.0.0.1
> > >> -ip,nw_src=10.0.0.2
> > >> -ip,nw_src=10.0.0.3
> > >> +ip,nw_src=10.0.0.2/31
> > >> +])
> > >> +AT_CHECK([expr_to_flow 'ip4.src == $set5'], [0], [dnl
> > >> +ip,nw_src=1.1.1.128/26
> > >> +ip,nw_src=1.1.1.16/28
> > >> +ip,nw_src=1.1.1.192/27
> > >> +ip,nw_src=1.1.1.2/31
> > >> +ip,nw_src=1.1.1.224/28
> > >> +ip,nw_src=1.1.1.240/29
> > >> +ip,nw_src=1.1.1.248/30
> > >> +ip,nw_src=1.1.1.252/31
> > >> +ip,nw_src=1.1.1.254
> > >> +ip,nw_src=1.1.1.32/27
> > >> +ip,nw_src=1.1.1.4/30
> > >> +ip,nw_src=1.1.1.64/26
> > >> +ip,nw_src=1.1.1.8/29
> > >> ])
> > >> AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0],
> [dnl
> > >> ip,nw_src=1.2.3.4
> > >> ip,nw_src=10.0.0.1
> > >> -ip,nw_src=10.0.0.2
> > >> -ip,nw_src=10.0.0.3
> > >> +ip,nw_src=10.0.0.2/31
> > >> ])
> > >> AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24,
> $set1}'], [0], [dnl
> > >> ip,nw_src=1.2.0.0/20
> > >> ip,nw_src=10.0.0.1
> > >> -ip,nw_src=10.0.0.2
> > >> -ip,nw_src=10.0.0.3
> > >> +ip,nw_src=10.0.0.2/31
> > >> ip,nw_src=5.5.5.0/24
> > >> ])
> > >> AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0],
> [dnl
> > >> @@ -13768,23 +13780,20 @@ cat 2.expected > expout
> > >> $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in"
> hv1/vif2-tx.pcap > 2.packets
> > >> AT_CHECK([cat 2.packets], [0], [expout])
> > >>
> > >> -# There should be total of 9 flows present with conjunction
> action and 2 flows
> > >> +# There should be total of 6 flows present with conjunction
> action and 2 flows
> > >> # with conj match. Eg.
> > >> -# table=44, priority=2001,conj_id=2,metadata=0x1
> actions=resubmit(,45)
> > >> -# table=44, priority=2001,conj_id=3,metadata=0x1
> actions=drop
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(2,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(2,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5
> actions=conjunction(2,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(3,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(3,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
> actions=conjunction(3,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(2,1/2),conjunction(3,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
> actions=conjunction(2,1/2),conjunction(3,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
> actions=conjunction(2,1/2),conjunction(3,1/2)
> > >> -
> > >> -OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +#table=45, n_packets=1, n_bytes=42, idle_age=211,
> priority=2001,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> > >> +#table=45, n_packets=0, n_bytes=0, idle_age=211,
> priority=2001,conj_id=3,ip,metadata=0x1 actions=drop
> > >> +#priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(2,2/2),conjunction(3,2/2)
> > >> +#priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31
> actions=conjunction(2,2/2),conjunction(3,2/2)
> > >> +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31
> actions=conjunction(2,1/2)
> > >> +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(2,1/2)
> > >> +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(3,1/2)
> > >> +#priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31
> actions=conjunction(3,1/2)
> > >> +
> > >> +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction | wc -l`])
> > >> -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction | wc -l`])
> > >> OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conj_id | wc -l`])
> > >> @@ -13811,18 +13820,16 @@ AT_CHECK([cat 2.packets], [0], [])
> > >> # properly.
> > >> # There should be total of 6 flows present with conjunction
> action and 1 flow
> > >> # with conj match. Eg.
> > >> -# table=44, priority=2001,conj_id=3,metadata=0x1
> actions=drop
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(4,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(4,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
> actions=conjunction(4,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(4,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
> actions=conjunction(4,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
> actions=conjunction(4,1/2)
> > >> +# table=45, n_packets=1, n_bytes=42, idle_age=34,
> priority=2001,conj_id=3,ip,metadata=0x1 actions=drop
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(3,2/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31
> actions=conjunction(3,2/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31
> actions=conjunction(3,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(3,1/2)
> > >>
> > >> ovn-nbctl acl-del ls1 to-lport 1001 \
> > >> 'ip4 && ip4.src == $set1 && ip4.dst == $set1'
> > >>
> > >> -OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction | wc -l`])
> > >> OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction | wc -l`])
> > >> @@ -13836,43 +13843,40 @@ ovn-nbctl acl-add ls1 to-lport
> 1001 \
> > >> ovn-nbctl acl-add ls1 to-lport 1001 \
> > >> 'ip4 && ip4.src == $set1 && ip4.dst == {10.0.0.9,
> 10.0.0.10}' drop
> > >>
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
> actions=conjunction(4,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(4,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(5,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5
> actions=conjunction(5,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(5,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(4,1/2),conjunction(6,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10
> actions=conjunction(6,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
> actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
> actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(4,2/2),conjunction(5,2/2),conjunction(6,2/2)
> > >> -
> > >> -OVS_WAIT_UNTIL([test 10 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
> actions=conjunction(3,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8/31
> actions=conjunction(3,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31
> actions=conjunction(4,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(4,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(5,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10
> actions=conjunction(5,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31
> actions=conjunction(3,2/2),conjunction(4,2/2),conjunction(5,2/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(3,2/2),conjunction(4,2/2),conjunction(5,2/2)
> > >> +
> > >> +OVS_WAIT_UNTIL([test 8 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction | wc -l`])
> > >> -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction | wc -l`])
> > >> -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction.*conjunction | wc -l`])
> > >>
> > >> # Remove 10.0.0.7 from address set2. All flows should be
> updated properly.
> > >> ovn-nbctl set Address_Set set2 \
> > >> addresses=\"10.0.0.8\",\"10.0.0.9\"
> > >>
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4
> actions=conjunction(9,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10
> actions=conjunction(7,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
> actions=conjunction(8,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5
> actions=conjunction(9,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(7,1/2),conjunction(8,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(9,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
> actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
> actions=conjunction(7,2/2),conjunction(8,2/2),conjunction(9,2/2)
> > >> -
> > >> -OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +#
> priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31,nw_dst=10.0.0.8/31
> actions=drop
> > >> +#
> priority=2001,ip,metadata=0x1,nw_src=10.0.0.6,nw_dst=10.0.0.8/31 actions=drop
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4/31
> actions=conjunction(4,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6
> actions=conjunction(4,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(5,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10
> actions=conjunction(5,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(5,2/2),conjunction(4,2/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31
> actions=conjunction(5,2/2),conjunction(4,2/2)
> > >> +
> > >> +OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction | wc -l`])
> > >> -OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction | wc -l`])
> > >> -OVS_WAIT_UNTIL([test 3 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction.*conjunction | wc -l`])
> > >>
> > >> # Remove an ACL again
> > >> @@ -13881,16 +13885,16 @@ ovn-nbctl acl-del ls1 to-lport
> 1001 \
> > >>
> > >> wait_for_ports_up
> > >> ovn-nbctl --wait=hv sync
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10
> actions=conjunction(10,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
> actions=conjunction(11,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(10,1/2),conjunction(11,1/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
> actions=conjunction(10,2/2),conjunction(11,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(10,2/2),conjunction(11,2/2)
> > >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
> actions=conjunction(10,2/2),conjunction(11,2/2)
> > >> +#
> priority=2001,ip,metadata=0x1,nw_src=10.0.0.6,nw_dst=10.0.0.8/31 actions=drop
> > >> +#
> priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31,nw_dst=10.0.0.8/31
> actions=drop
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
> actions=conjunction(5,2/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4/31
> actions=conjunction(5,2/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.10
> actions=conjunction(5,1/2)
> > >> +# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
> actions=conjunction(5,1/2)
> > >>
> > >> -OVS_WAIT_UNTIL([test 6 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> -grep conjunction | wc -l`])
> > >> OVS_WAIT_UNTIL([test 4 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> +grep conjunction | wc -l`])
> > >> +OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction | wc -l`])
> > >> OVS_WAIT_UNTIL([test 0 = `as hv1 ovs-ofctl dump-flows
> br-int | \
> > >> grep conjunction.*conjunction.*conjunction | wc -l`])
> > >> @@ -15549,11 +15553,11 @@ for i in 1 2 3; do
> > >>
> > >> # Update address set as1
> > >> ovn-nbctl --wait=hv set addr as1 addresses="10.1.2.10
> 10.1.2.11"
> > >> - AT_CHECK([ovs-ofctl dump-flows br-int | grep
> "10.1.2.11"], [0], [ignore])
> > >> + AT_CHECK([ovs-ofctl dump-flows br-int | grep
> "10.1.2.10/31"], [0], [ignore])
> > >>
> > >> # Update address set as2
> > >> ovn-nbctl --wait=hv set addr as2 addresses="10.1.2.12
> 10.1.2.13"
> > >> - AT_CHECK([ovs-ofctl dump-flows br-int | grep
> "10.1.2.12"], [0], [ignore])
> > >> + AT_CHECK([ovs-ofctl dump-flows br-int | grep
> "10.1.2.12/31"], [0], [ignore])
> > >>
> > >> # Add another ACL referencing as1
> > >> n_flows_before=`ovs-ofctl dump-flows br-int | grep
> "10.1.2.10" | wc -l`
> > >> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > >> index 202a96c5d..4c13bacda 100644
> > >> --- a/tests/test-ovn.c
> > >> +++ b/tests/test-ovn.c
> > >> @@ -227,10 +227,19 @@ create_addr_sets(struct shash
> *addr_sets)
> > >> };
> > >> static const char *const addrs4[] = { NULL };
> > >>
> > >> + static const char *addrs5[253];
> > >> + static char addrs5_[253][20];
> > >> + for (int i = 0, ip_num = 2; i < 253; i++, ip_num++) {
> > >> + sprintf(addrs5_[i], "1.1.1.%d", ip_num);
> > >> + addrs5[i] = addrs5_[i];
> > >> + }
> > >> +
> > >> expr_const_sets_add(addr_sets, "set1", addrs1, 3, true);
> > >> expr_const_sets_add(addr_sets, "set2", addrs2, 3, true);
> > >> expr_const_sets_add(addr_sets, "set3", addrs3, 3, true);
> > >> expr_const_sets_add(addr_sets, "set4", addrs4, 0, true);
> > >> + expr_const_sets_add(addr_sets, "set5", (const char
> *const *)addrs5,
> > >> + 253, true);
> > >> }
> > >>
> > >> static void
> > >> diff --git a/utilities/automake.mk b/utilities/automake.mk
> > >> index c4a6d248c..af572c7a0 100644
> > >> --- a/utilities/automake.mk
> > >> +++ b/utilities/automake.mk
> > >> @@ -21,7 +21,8 @@ MAN_ROOTS += \
> > >> bin_SCRIPTS += \
> > >> utilities/ovn-docker-overlay-driver \
> > >> utilities/ovn-docker-underlay-driver \
> > >> - utilities/ovn-detrace
> > >> + utilities/ovn-detrace \
> > >> + utilities/ovn-match-ip
> > >>
> > >> EXTRA_DIST += \
> > >> utilities/ovn-ctl \
> > >> @@ -35,6 +36,7 @@ EXTRA_DIST += \
> > >> utilities/ovn-appctl.8.xml \
> > >> utilities/ovn-trace.8.xml \
> > >> utilities/ovn-detrace.in \
> > >> + utilities/ovn-match-ip.in \
> > >> utilities/ovndb-servers.ocf \
> > >> utilities/checkpatch.py \
> > >> utilities/docker/Makefile \
> > >> @@ -60,6 +62,7 @@ CLEANFILES += \
> > >> utilities/ovn-trace.8 \
> > >> utilities/ovn-detrace.1 \
> > >> utilities/ovn-detrace \
> > >> + utilities/ovn-match-ip \
> > >> utilities/ovn-appctl.8 \
> > >> utilities/ovn-appctl \
> > >> utilities/ovn-sim
> > >> diff --git a/utilities/ovn-match-ip.in
> b/utilities/ovn-match-ip.in
> > >> new file mode 100755
> > >> index 000000000..af8124185
> > >> --- /dev/null
> > >> +++ b/utilities/ovn-match-ip.in
> > >> @@ -0,0 +1,78 @@
> > >> +#!/bin/bash
> > >> +#
> > >> +# Licensed under the Apache License, Version 2.0 (the
> "License");
> > >> +# you may not use this file except in compliance with the
> License.
> > >> +# You may obtain a copy of the License at:
> > >> +#
> > >> +# http://www.apache.org/licenses/LICENSE-2.0
> > >> +#
> > >> +# Unless required by applicable law or agreed to in
> writing, software
> > >> +# distributed under the License is distributed on an "AS
> IS" BASIS,
> > >> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
> express or implied.
> > >> +# See the License for the specific language governing
> permissions and
> > >> +# limitations under the License.
> > >> +
> > >> +IPCALC_WILDCARD_RET=
> > >> +
> > >> +ipcalc_wildcard () {
> > >> + ip=`echo $1 | awk -F '/' '{print $1}'`
> > >> + ip_list=${ip//./ }
> > >> + read -a ip_array <<< ${ip_list}
> > >> + mask=`echo $1 | awk -F '/' '{print $2}'`
> > >> + mask_list=${mask//./ }
> > >> + read -a mask_array <<< ${mask_list}
> > >> + ip_0=$((${ip_array[0]} & ${mask_array[0]}))
> > >> + ip_1=$((${ip_array[1]} & ${mask_array[1]}))
> > >> + ip_2=$((${ip_array[2]} & ${mask_array[2]}))
> > >> + ip_3=$((${ip_array[3]} & ${mask_array[3]}))
> > >> + IPCALC_WILDCARD_RET="$ip_0.$ip_1.$ip_2.$ip_3"
> > >> +}
> > >> +
> > >> +for arg
> > >> +do
> > >> + case $arg in
> > >> + -h | --help)
> > >> + cat <<EOF
> > >> +$0: A tool that can match ip wildcard mask similar to grep
> > >> +usage: $0 ip
> > >> +Search for ip/mask that can be matched in standard input.
> > >> +Example: echo ip 192.168.1.0/24 | $0 192.168.1.1
> > >> +Result: ip 192.168.1.0/24
> > >> +
> > >> +You might use this: ovs-ofctl dump-flows br-int | $0
> 192.168.1.1
> > >> +EOF
> > >> + exit 0
> > >> + ;;
> > >> + esac
> > >> +done
> > >> +
> > >> +while read line
> > >> +do
> > >> + ip=`echo $line | grep -oE
> '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+'`
> > >> + if test "$ip" == "$1"; then
> > >> + echo $line | grep --color $1
> > >> + continue
> > >> + fi
> > >> +
> > >> + ip_mask=`echo $line | grep -oE
> '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+'`
> > >> + if test "$ip_mask" != ""; then
> > >> + ipcalc_wildcard $ip_mask
> > >> + ip_mask_ret=$IPCALC_WILDCARD_RET
> > >> + match_ip_mask=`echo $ip_mask | sed 's/.*\//'$1'\//'`
> > >> + ipcalc_wildcard $match_ip_mask
> > >> + match_ip_mask_ret=$IPCALC_WILDCARD_RET
> > >> + if test $ip_mask_ret == $match_ip_mask_ret; then
> > >> + echo $line | grep --color $ip_mask
> > >> + fi
> > >> + continue
> > >> + fi
> > >> +
> > >> + ip_prefix=`echo $line | grep -oE
> '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+'`
> > >> + if test "$ip_prefix" != ""; then
> > >> + match_ip_prefix=`echo $ip_prefix | sed
> 's/.*\//'$1'\//'`
> > >> + if test `ipcalc -n $match_ip_prefix` == `ipcalc -n
> $ip_prefix`; then
> > >> + echo $line | grep --color $ip_prefix
> > >> + fi
> > >> + continue
> > >> + fi
> > >> +done
> > >>
> > >
> > > Regards,
> > > Dumitru
> > >
> > > _______________________________________________
> > > 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
