D17185: Added ip-tunnel settings

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> CMakeLists.txt:31
>      wirelesssecuritysettingtest
> +    iptunnelsettingtest
>  )

Can you please add the test in alphabetic order?

> iptunnelsettingtest.cpp:79
> +
> +    map.insert(QLatin1String(NMQT_SETTING_IP_TUNNEL_CONFIG_MODE), mode);
> +    map.insert(QLatin1String(NMQT_SETTING_IP_TUNNEL_CONFIG_PATH_MTU_DISCOVERY), pathMtuDiscovery);

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

> iptunnelsettingtest.h:2
> +/*
> +    Copyright 2016 Jan Grulich <jgrulich at redhat.com>
> +

You should add yourself here.

> CMakeLists.txt:229
>    WirelessSetting
> +  IpTunnelSetting
>  

Please add this in alphabetic order.

> connectionsettings.cpp:195
>          addSetting(Setting::Ptr(new Ipv6Setting()));
> +        addSetting(Setting::Ptr(new IpTunnelSetting()));
>          break;

IpTunnel setting is not part of Tun connection, it should be completely separated connection type.

> iptunnelsetting.cpp:27
> +NetworkManager::IpTunnelSettingPrivate::IpTunnelSettingPrivate()
> +: name(NM_SETTING_IP_TUNNEL_SETTING_NAME)
> +, mode(IpTunnelSetting::DefaultMode)

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

> iptunnelsetting.h:29
> +
> +#define NMQT_SETTING_IP_TUNNEL_CONFIG_ENCAPSULATION_LIMIT   NM_SETTING_IP_TUNNEL_ENCAPSULATION_LIMIT
> +#define NMQT_SETTING_IP_TUNNEL_CONFIG_FLAGS                 "flags"//NM_SETTING_IP_TUNNEL_FLAGS

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

> iptunnelsetting.h:57
> +    typedef QList<Ptr> List;
> +    enum Mode { DefaultMode, Ipip, Gre };
> +

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;

> iptunnelsetting.h:74
> +
> +    void setFlags(quint32 flags);
> +    quint32 flags() const;

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;

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17185

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181128/f939b3ee/attachment.html>