Added match and tc setting according to:
https://developer.gnome.org/NetworkManager/stable/settings-match.html https://developer.gnome.org/NetworkManager/stable/settings-tc.html
Details
Details
- Reviewers
jgrulich - Commits
- R282:e95bb9bdf98c: match and tc setting
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| autotests/settings/matchsettingtest.cpp | ||
|---|---|---|
| 44 | This define won't exist for NM < 1.12.0 | |
| autotests/settings/tcsettingtest.cpp | ||
| 59 | TC defines do not exist before NM 1.10.0 | |
| 70 | I think that you cannot compare NMVariantMaps this way, that's why probably the test for IPv4 and IPv6 didn't fail when we swapped values for route-data and address-data. You will need to compare values inside the maps separately. | |
| src/settings/matchsetting.h | ||
| 28 | I think you don't need to include <QString> | |
| 30 | Move those defines out of the header file. | |
| src/settings/tcsetting.cpp | ||
| 116 | const QVariantMap &qdisc | |
| 119 | Indent. | |
| 123 | const QVariantMap &tfilter | |
| src/settings/tcsetting.h | ||
| 31 | TC defines are present since NM 1.10.0. Also move them out of the header file. | |
| src/settings/tcsetting_p.h | ||
| 26 | This include shouldn't be needed. | |
| autotests/settings/tcsettingtest.cpp | ||
|---|---|---|
| 70 | So..how should I do it? | |
| autotests/settings/tcsettingtest.cpp | ||
|---|---|---|
| 70 | See below, it's ugly, but problem is that give it's a QList, the maps can be in different order so you first have to identify you compare correct QVariantMaps. I do that by comparing keys. | |
| 75 | Something like: NMVariantMapList list = it.value();
NMVariantMapList list1 = map1.value(it.key());
QCOMPARE(list.count(), list1.count());
int comparedMaps = 0;
NMVariantMapList::const_iterator listIt = list.constBegin();
while (listIt != list.constEnd() {
NMVariantMapList::const_iterator list1It = list1.constBegin();
while (list1it != list1.constEnd()) {
QVariantMap listMap = listIt.value();
QVariantMap1 listMap1 = list1It.value();
// Test if keys do match, because the list can be in different order
QStringList listMapKeys = listMap.keys();
QStringList listMapKeys1 = listMap1.keys();
listMapKeys.sort();
listMapKeys1.sort();
if (listMapKeys.join(QChar(' ')) == listMapKeys1.join(QChar(' '))) {
// Here the maps should have same keys so compare QVariantMaps as we do now
.........
++comparedMaps;
}
++list1it;
}
++listIt;
}
// Test if we compared all maps, if not, then probably they didn't match
QCOMPARE(comparedMaps, list.count(); | |
| autotests/settings/matchsettingtest.cpp | ||
|---|---|---|
| 30 | NM 1.14.0, which is not released yet. | |
| autotests/settings/tcsettingtest.cpp | ||
| 75 | It should be: // Will fail if set some default values, because they are skipped in toMap() method
QVariantMap::const_iterator it = map.constBegin();
while (it != map.constEnd()) {
NMVariantMapList list = it.value().value<NMVariantMapList>();
NMVariantMapList list1 = map1.value(it.key()).value<NMVariantMapList>();
QCOMPARE(list.count(), list1.count());
int comparedMaps = 0;
NMVariantMapList::const_iterator listIt = list.constBegin();
while (listIt != list.constEnd()) {
NMVariantMapList::const_iterator list1It = list1.constBegin();
while (list1It != list1.constEnd()) {
QVariantMap listMap = *listIt;
QVariantMap listMap1 = *list1It;
// Test if keys do match, because the list can be in different order
QStringList listMapKeys = listMap.keys();
QStringList listMapKeys1 = listMap1.keys();
listMapKeys.sort();
listMapKeys1.sort();
if (listMapKeys.join(QChar(' ')) == listMapKeys1.join(QChar(' '))) {
// Here the maps should have same keys so compare QVariantMaps as we do now
.........
++comparedMaps;
}
++list1It;
}
++listIt;
}
++it;
}You just need to add missing comparison of variant maps, which should be trivial. | |
| src/settings/matchsetting.cpp | ||
| 27 | This is actually in master only, which means upcoming NM 1.14.0. | |
| src/settings/setting.cpp | ||
| 33 | Same here, should be NM 1.14.0. | |
Comment Actions
And you submitted only changes on top of your changes.
| autotests/settings/tcsettingtest.cpp | ||
|---|---|---|
| 92 ↗ | (On Diff #46789) | I liked your original solution better, I mean the way you compared if you are going to compare correct maps. Anyway, this still doesn't compare the maps properly, you have to go value by value. |
Comment Actions
Can you please rebase your changes on top of the current master? I did some changes there and the patch is not applicable.
Comment Actions
Because it's broken in numerous ways :).
void TcSettingTest::testSetting()
{
QFETCH(NMVariantMapList, tfilters);
QFETCH(NMVariantMapList, qdiscs);
QVariantMap map;
map.insert(QLatin1String(NM_SETTING_TC_CONFIG_TFILTERS), QVariant::fromValue(tfilters));
map.insert(QLatin1String(NM_SETTING_TC_CONFIG_QDISCS), QVariant::fromValue(qdiscs));
NetworkManager::TcSetting setting;
setting.fromMap(map);
QVariantMap map1 = setting.toMap();
QVariantMap::const_iterator it = map.constBegin();
while (it != map.constEnd()) {
NMVariantMapList list = it.value().value<NMVariantMapList>();
NMVariantMapList list1 = map1.value(it.key()).value<NMVariantMapList>();
QCOMPARE(list.count(), list1.count());
int comparedMaps = 0;
for (int i = 0; i < list.size(); ++i) {
QVariantMap varMap = list.at(i);
for (int j = 0; j < list1.size(); ++j) {
QVariantMap varMap1 = list1.at(i);
QVariantMap::const_iterator ite = varMap.constBegin();
int comparedvals = 0;
while (ite != varMap.constEnd()) {
QVariantMap::const_iterator val1 = varMap1.constFind(ite.key());
if (val1 != varMap1.constEnd()) {
if (varMap.value(ite.key()) == val1.value()) {
++comparedvals;
}
}
++ite;
}
if (comparedvals == varMap.size()) {
comparedMaps++;
}
}
}
++it;
QCOMPARE(comparedMaps, list.count());
}
}It should be like this.