Added ip-tunnel settings
ClosedPublic

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

Details

Reviewers
jgrulich
Summary

Diff Detail

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

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:
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;

75

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
80

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
75

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
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.

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.

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.

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
3

Here should be your name.

src/settings/connectionsettings.h
70

Remove the comment.

src/settings/iptunnelsetting.cpp
28

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
38

Coding style.

44

Coding style.

182

Coding style.

196

Same here.

210

Same here.

224

Same here.

238

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
38

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.

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
30

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.

47

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