Added ip-tunnel settings according to https://developer.gnome.org/NetworkManager/stable/settings-ip-tunnel.html
Details
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
autotests/settings/CMakeLists.txt | ||
---|---|---|
31 | Can you please add the test in alphabetic order? | |
autotests/settings/iptunnelsettingtest.cpp | ||
80 | Use NetworkManager defines, do not define your own new defines, there is no reason for that. | |
autotests/settings/iptunnelsettingtest.h | ||
3 | You should add yourself here. | |
src/CMakeLists.txt | ||
229 | Please add this in alphabetic order. | |
src/settings/connectionsettings.cpp | ||
195 | IpTunnel setting is not part of Tun connection, it should be completely separated connection type. | |
src/settings/iptunnelsetting.cpp | ||
28 | Coding style, missing indentation. This applies also for lines below. | |
src/settings/iptunnelsetting.h | ||
30 | There is no reason to create your own defines, use just those NM_SETTING_IP_TUNNEL_PROPERTY_NAME. | |
58 | There are other modes, they are just not documented in the documentation, but can be found in the source code. See: | |
75 | Maybe turn this into QFlags? See: |
autotests/settings/iptunnelsettingtest.cpp | ||
---|---|---|
80 | Only IPv4Setting and IPv6Setting do that this way, other setting classes don't. | |
src/settings/connectionsettings.cpp | ||
195 | You just add it the same way as for example this Tun connection type is addded. | |
src/settings/iptunnelsetting.h | ||
75 | Check QFlags, it's similar to enum, but you can combine them. |
You added just new changes you did on top of your original review, you need to include full patch, also with previous changes.
autotests/settings/iptunnelsettingtest.cpp | ||
---|---|---|
81 | This seems to be wrong. | |
src/settings/connectionsettings.cpp | ||
197 | You need to add also IPv4 and IPv6 settings. | |
318 | Same here. | |
src/settings/connectionsettings.h | ||
70 | IpTunnel is correct name. | |
src/settings/iptunnelsetting.cpp | ||
26 | You also need to define NM_SETTING_IP_TUNNEL_FLAGS in case NM is older than 1.12. | |
28 | Coding style still. | |
src/settings/iptunnelsetting.h | ||
60 | Turn this into QFlags. You can check how this should be done for example in VlanSetting | |
75 | Coding style. It should be setFoo(const QString &foo), the '&' should be separated from QString by a space and not separated by a space from "key". Same for other methods below. |
src/settings/iptunnelsetting.cpp | ||
---|---|---|
37 | Still misses indentation and code below as well. | |
src/settings/iptunnelsetting.h | ||
30 | This shouldn't be commented out. | |
47 | There is no default mode, the first one should be Unknown. Also you maybe want to assign values to them? Like: enum Mode { Unknown = NM_IP_TUNNEL_MODE_UNKNOWN, Ipip = NM_IP_TUNNEL_MODE_IPIP etc. } | |
48 | The flags don't seem to use full name. For example NM_IP_TUNNEL_FLAG_IP6_IGN_ENCAP_LIMIT should perhabs be IgnEncapLimit. |