Skip to content

Commit d8cb377

Browse files
jengelhjengelh
committed
mapi_lib: fix broken idset range overlap merging, and improve adjacent merging
Fixes: gromox-0~666
1 parent 66fd0d2 commit d8cb377

File tree

3 files changed

+161
-29
lines changed

3 files changed

+161
-29
lines changed

‎Makefile.am‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ tests_cryptest_LDADD = libgromox_common.la
287287
tests_icalparse_SOURCES = tests/icalparse.cpp
288288
tests_icalparse_LDADD = ${HX_LIBS} libgromox_common.la libgromox_email.la libgromox_mapi.la
289289
tests_utiltest_SOURCES = tests/utiltest.cpp
290-
tests_utiltest_LDADD = libgromox_common.la
290+
tests_utiltest_LDADD = libgromox_common.la libgromox_mapi.la
291291
tests_zendfake_SOURCES = tests/zendfake.cpp
292292
tests_zendfake_LDADD = libmapi4zf.la
293293

‎lib/mapi/idset.cpp‎

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -134,34 +134,46 @@ BOOL idset::append_range(uint16_t replid,
134134
if (prepl_node == repl_list.end())
135135
prepl_node = repl_list.emplace(repl_list.end(), replid);
136136
auto &range_list = prepl_node->range_list;
137-
auto prange_node1 = range_list.end();
138-
for (auto prange_node = range_list.begin();
139-
prange_node != range_list.end(); ++prange_node) {
140-
if (prange_node1 == range_list.end()) {
141-
auto succ = std::next(prange_node);
142-
if (low_value == prange_node->high_value) {
143-
prange_node1 = prange_node;
144-
prange_node1->high_value = high_value;
145-
} else if (low_value > prange_node->high_value &&
146-
(succ == range_list.end() || high_value <= succ->low_value)) {
147-
prange_node = prange_node1 = range_list.emplace(prange_node, low_value, high_value);
148-
}
149-
continue;
150-
}
151-
if (high_value == prange_node->low_value) {
152-
prange_node1->high_value = prange_node->high_value;
153-
range_list.erase(prange_node);
154-
return TRUE;
155-
} else if (high_value < prange_node->low_value) {
156-
return TRUE;
137+
auto iter = range_list.begin();
138+
bool merge = false;
139+
/*
140+
* If newrange fills a gap between iter->high_value and
141+
* std::next(iter)->low_value precisely, then it will simply be
142+
* right-adjacent to iter now and gets merged; thus never showing up as
143+
* left-adjacent in subsequent iterations (not that there is any more
144+
* iteration - the loop exits anyway).
145+
*/
146+
for (; iter != range_list.end(); ++iter) {
147+
bool nr_is_after = low_value > iter->high_value + 1;
148+
bool nr_is_before = high_value + 1 < iter->low_value;
149+
merge = !nr_is_after && !nr_is_before;
150+
if (merge) {
151+
iter->low_value = std::min(iter->low_value, low_value);
152+
iter->high_value = std::max(iter->high_value, high_value);
153+
break;
157154
}
158-
prange_node = range_list.erase(prange_node);
159-
if (prange_node == range_list.end())
160-
return TRUE;
155+
if (nr_is_before)
156+
break;
161157
}
162-
if (prange_node1 != range_list.end())
158+
if (!merge) {
159+
/* newrange is non-adjacent non-overlapping. */
160+
range_list.emplace(iter, low_value, high_value);
163161
return TRUE;
164-
range_list.emplace_back(low_value, high_value);
162+
}
163+
/*
164+
* When a merge happened, new adjacencies/overlaps could have formed.
165+
* Because of the property that no left adjacencies could have been
166+
* introduced (see above), we only need to check to the right of @iter,
167+
* and also only on the high side.
168+
*/
169+
auto bigone = iter;
170+
for (++iter; iter != range_list.end(); iter = range_list.erase(iter)) {
171+
auto ext_right = iter->high_value > bigone->high_value &&
172+
iter->low_value <= bigone->high_value + 1;
173+
if (!ext_right)
174+
break;
175+
bigone->high_value = iter->high_value;
176+
}
165177
return TRUE;
166178
} catch (const std::bad_alloc &) {
167179
fprintf(stderr, "E-1614: ENOMEM\n");

‎tests/utiltest.cpp‎

Lines changed: 123 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,122 @@
11
#include <cassert>
22
#include <cstdio>
33
#include <cstdlib>
4+
#include <gromox/mapi_types.hpp>
5+
#include <gromox/rop_util.hpp>
46
#include <gromox/util.hpp>
57
using namespace gromox;
8+
9+
static int t_id1()
10+
{
11+
idset s(true, REPL_TYPE_ID);
12+
/* right-side half overlap */
13+
s.append_range(1, 1, 17);
14+
s.append_range(1, 14, 21);
15+
auto &l = s.repl_list.front().range_list;
16+
assert(l.size() == 1);
17+
assert(l.front().low_value == 1);
18+
assert(l.front().high_value == 21);
19+
return EXIT_SUCCESS;
20+
}
21+
22+
static int t_id2()
23+
{
24+
idset s(true, REPL_TYPE_ID);
25+
/* left-side half overlap */
26+
s.append_range(1, 14, 21);
27+
s.append_range(1, 1, 17);
28+
auto &l = s.repl_list.front().range_list;
29+
assert(l.size() == 1);
30+
assert(l.front().low_value == 1);
31+
assert(l.front().high_value == 21);
32+
return EXIT_SUCCESS;
33+
}
34+
35+
static int t_id3()
36+
{
37+
idset s(true, REPL_TYPE_ID);
38+
/* right-side adjacency */
39+
s.append_range(1, 1, 19);
40+
s.append_range(1, 20, 29);
41+
auto &l = s.repl_list.front().range_list;
42+
assert(l.size() == 1);
43+
assert(l.front().low_value == 1);
44+
assert(l.front().high_value == 29);
45+
return EXIT_SUCCESS;
46+
}
47+
48+
static int t_id4()
49+
{
50+
idset s(true, REPL_TYPE_ID);
51+
/* left-side adjacency */
52+
s.append_range(1, 20, 29);
53+
s.append_range(1, 1, 19);
54+
auto &l = s.repl_list.front().range_list;
55+
assert(l.size() == 1);
56+
assert(l.front().low_value == 1);
57+
assert(l.front().high_value == 29);
58+
return EXIT_SUCCESS;
59+
}
60+
61+
static int t_id5()
62+
{
63+
idset s(true, REPL_TYPE_ID);
64+
/* inner overlap */
65+
s.append_range(1, 1, 40);
66+
s.append_range(1, 15, 19);
67+
auto &l = s.repl_list.front().range_list;
68+
assert(l.size() == 1);
69+
assert(l.front().low_value == 1);
70+
assert(l.front().high_value == 40);
71+
return EXIT_SUCCESS;
72+
}
73+
74+
static int t_id6()
75+
{
76+
idset s(true, REPL_TYPE_ID);
77+
/* outer overlap */
78+
s.append_range(1, 15, 19);
79+
s.append_range(1, 1, 40);
80+
auto &l = s.repl_list.front().range_list;
81+
assert(l.size() == 1);
82+
assert(l.front().low_value == 1);
83+
assert(l.front().high_value == 40);
84+
return EXIT_SUCCESS;
85+
}
86+
87+
static int t_id7()
88+
{
89+
idset s(true, REPL_TYPE_ID);
90+
s.append_range(1, 11, 19);
91+
s.append_range(1, 71, 79);
92+
auto &l = s.repl_list.front().range_list;
93+
assert(l.size() == 2);
94+
assert(l.front().low_value == 11);
95+
assert(l.front().high_value == 19);
96+
assert(l.back().low_value == 71);
97+
assert(l.back().high_value == 79);
98+
s.append_range(1, 41, 49);
99+
assert(l.size() == 3);
100+
assert(l.front().low_value == 11);
101+
assert(l.front().high_value == 19);
102+
assert(std::next(l.begin())->low_value == 41);
103+
assert(std::next(l.begin())->high_value == 49);
104+
assert(l.back().low_value == 71);
105+
assert(l.back().high_value == 79);
106+
/* gap filler */
107+
s.append_range(1, 20, 40);
108+
assert(l.size() == 2);
109+
assert(l.front().low_value == 11);
110+
assert(l.front().high_value == 49);
111+
assert(l.back().low_value == 71);
112+
assert(l.back().high_value == 79);
113+
s.append_range(1, 50, 70);
114+
assert(l.size() == 1);
115+
assert(l.front().low_value == 11);
116+
assert(l.front().high_value == 79);
117+
return EXIT_SUCCESS;
118+
}
119+
6120
static int t_interval()
7121
{
8122
const char *in = " 1 d 1 h 1 m 1 s ";
@@ -14,9 +128,15 @@ static int t_interval()
14128
}
15129
return EXIT_SUCCESS;
16130
}
131+
17132
int main()
18133
{
19-
auto ret = t_interval();
20-
if (ret != EXIT_SUCCESS)
21-
return ret;
134+
using fpt = decltype(&t_interval);
135+
fpt fct[] = {t_interval, t_id1, t_id2, t_id3, t_id4, t_id5, t_id6, t_id7};
136+
for (auto f : fct) {
137+
auto ret = f();
138+
if (ret != EXIT_SUCCESS)
139+
return ret;
140+
}
141+
return EXIT_SUCCESS;
22142
}

0 commit comments

Comments
 (0)