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