Added ip-tunnel settings
ClosedPublic

Authored by pranavgade on Nov 27 2018, 1:24 PM.

Details

Reviewers
jgrulich
Summary

Diff Detail

Repository
R282 NetworkManagerQt
Lint
Lint Skipped
Unit
Unit Tests Skipped
pranavgade created this revision.Nov 27 2018, 1:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 27 2018, 1:24 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pranavgade requested review of this revision.Nov 27 2018, 1:24 PM
jgrulich added inline comments.Nov 28 2018, 7:43 AM
autotests/settings/CMakeLists.txt
31 ↗(On Diff #46310)

Can you please add the test in alphabetic order?

autotests/settings/iptunnelsettingtest.cpp
79

Use NetworkManager defines, do not define your own new defines, there is no reason for that.

autotests/settings/iptunnelsettingtest.h
2 ↗(On Diff #46310)

You should add yourself here.

src/CMakeLists.txt
229 ↗(On Diff #46310)

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
27

Coding style, missing indentation. This applies also for lines below.

src/settings/iptunnelsetting.h
29

There is no reason to create your own defines, use just those NM_SETTING_IP_TUNNEL_PROPERTY_NAME.

57

There are other modes, they are just not documented in the documentation, but can be found in the source code.

See:
typedef enum {
NM_IP_TUNNEL_MODE_UNKNOWN = 0,
NM_IP_TUNNEL_MODE_IPIP = 1,
NM_IP_TUNNEL_MODE_GRE = 2,
NM_IP_TUNNEL_MODE_SIT = 3,
NM_IP_TUNNEL_MODE_ISATAP = 4,
NM_IP_TUNNEL_MODE_VTI = 5,
NM_IP_TUNNEL_MODE_IP6IP6 = 6,
NM_IP_TUNNEL_MODE_IPIP6 = 7,
NM_IP_TUNNEL_MODE_IP6GRE = 8,
NM_IP_TUNNEL_MODE_VTI6 = 9,
} NMIPTunnelMode;

74

Maybe turn this into QFlags?

See:
typedef enum { /*< flags, prefix=NM_IP_TUNNEL_FLAG >*/
NM_IP_TUNNEL_FLAG_NONE = 0x0,
NM_IP_TUNNEL_FLAG_IP6_IGN_ENCAP_LIMIT = 0x1,
NM_IP_TUNNEL_FLAG_IP6_USE_ORIG_TCLASS = 0x2,
NM_IP_TUNNEL_FLAG_IP6_USE_ORIG_FLOWLABEL = 0x4,
NM_IP_TUNNEL_FLAG_IP6_MIP6_DEV = 0x8,
NM_IP_TUNNEL_FLAG_IP6_RCV_DSCP_COPY = 0x10,
NM_IP_TUNNEL_FLAG_IP6_USE_ORIG_FWMARK = 0x20,
} NMIPTunnelFlags;

pranavgade marked 5 inline comments as done.Nov 28 2018, 8:08 AM
pranavgade added inline comments.
autotests/settings/iptunnelsettingtest.cpp
79

I tried to follow the way it is done in ipv6settings.

src/settings/connectionsettings.cpp
195

What should the new connection type be called?

src/settings/iptunnelsetting.h
74

What do you mean? Should I create a new Enum?

pranavgade updated this revision to Diff 46376.Nov 28 2018, 8:13 AM
pranavgade removed a subscriber: kde-frameworks-devel.

Made a few minor changes

jgrulich added inline comments.Nov 28 2018, 8:14 AM
autotests/settings/iptunnelsettingtest.cpp
79

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
74

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.

pranavgade updated this revision to Diff 46380.Nov 28 2018, 8:35 AM
pranavgade marked an inline comment as done.
pranavgade updated this revision to Diff 46381.Nov 28 2018, 8:37 AM

A few minor updates

jgrulich added inline comments.Nov 28 2018, 8:46 AM
autotests/settings/iptunnelsettingtest.cpp
81

This seems to be wrong.

src/settings/connectionsettings.cpp
197

You need to add also IPv4 and IPv6 settings.

316

Same here.

src/settings/connectionsettings.h
70

IpTunnel is correct name.

src/settings/iptunnelsetting.cpp
25

You also need to define NM_SETTING_IP_TUNNEL_FLAGS in case NM is older than 1.12.

27

Coding style still.

src/settings/iptunnelsetting.h
59

Turn this into QFlags. You can check how this should be done for example in VlanSetting

74

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.

pranavgade updated this revision to Diff 46384.Nov 28 2018, 9:15 AM
pranavgade marked 5 inline comments as done.

Made the changes required

pranavgade updated this revision to Diff 46386.Nov 28 2018, 9:22 AM

added missing indents

jgrulich added inline comments.Nov 28 2018, 9:22 AM
autotests/settings/iptunnelsettingtest.h
2 ↗(On Diff #46384)

Here should be your name.

src/settings/connectionsettings.h
70

Remove the comment.

src/settings/iptunnelsetting.cpp
27

You still don't follow KDE/Qt coding style. Check other classes to see how the indentation should be done.

jgrulich added inline comments.Nov 28 2018, 9:24 AM
src/settings/iptunnelsetting.cpp
37

Coding style.

43

Coding style.

181

Coding style.

195

Same here.

209

Same here.

223

Same here.

237

And same here.

pranavgade updated this revision to Diff 46387.Nov 28 2018, 9:28 AM
pranavgade marked 4 inline comments as done.

Updated code to follow coding style

jgrulich added inline comments.Nov 28 2018, 9:36 AM
src/settings/iptunnelsetting.cpp
37

Still misses indentation and code below as well.

src/settings/iptunnelsetting.h
29

This shouldn't be commented out.

46

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.

}

47

The flags don't seem to use full name. For example NM_IP_TUNNEL_FLAG_IP6_IGN_ENCAP_LIMIT should perhabs be IgnEncapLimit.

pranavgade updated this revision to Diff 46389.Nov 28 2018, 9:46 AM
pranavgade marked 14 inline comments as done.

updated code

pranavgade marked 8 inline comments as done.Nov 28 2018, 9:56 AM
jgrulich added inline comments.Nov 28 2018, 10:18 AM
src/settings/iptunnelsetting.h
29

This should be actually !NM_CHECK_VERSION(1, 12 0), because we want to define it for NM < 1.12.0. Sorry I didn't spot this before.

46

The change you did for flags was meant to be for Mode enum. The first flag is None, that was correct.

pranavgade marked 2 inline comments as done.
jgrulich accepted this revision.Nov 28 2018, 10:46 AM
This revision is now accepted and ready to land.Nov 28 2018, 10:46 AM
jgrulich closed this revision.Nov 28 2018, 10:57 AM