D15093: Add WireGuard capability.
Pino Toscano
noreply at phabricator.kde.org
Sun Sep 9 23:10:40 BST 2018
pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.
Thanks Bruce for all the updates, and patience! In return, I have more notes :)
- I'd send the changes to the existing SimpleIpV4AddressValidator in an own review request, since it affects other code than just this new wireguard plugin (maybe with unit tests)
- I'd send the addition of SimpleIpListValidator in an own review request too, since it is an independent code (possibly with unit tests)
- something I forgot (sorry) in the past: `QRegExp` is the "old" class for regexps, while in new code `QRegularExpression` should be preferred/used; should be mostly a transparent change for your uses
- I notes various nits for the UI strings: this is to follow the HIG, see https://hig.kde.org/style/writing/index.html in particular
- if possible, please avoid HTML markup in strings in UI files -- they make translations slightly more complicated (e.g. more prone to break the string with a typo in the markup)
INLINE COMMENTS
> wireguard.cpp:119
> + valueList = interfaceGroup.readEntry(NMV_WG_TAG_ADDRESS, QStringList());
> + if (valueList.size() == 0)
> + return result;
isEmpty()
> wireguard.cpp:121
> + return result;
> + for (int i = 0; i < valueList.size(); i++) {
> + QPair<QHostAddress, int> addressIn = QHostAddress::parseSubnet(valueList[i].trimmed());
better use qt's foreach here:
Q_FOREACH (const QString &address, valueList)
and then using `address` instead of `valueList[i]`
> wireguard.cpp:122
> + for (int i = 0; i < valueList.size(); i++) {
> + QPair<QHostAddress, int> addressIn = QHostAddress::parseSubnet(valueList[i].trimmed());
> + if (addressIn.first.protocol() == QAbstractSocket::NetworkLayerProtocol::IPv4Protocol)
addressIn can be const
> wireguard.cpp:134-135
> + if (!value.isEmpty()) {
> + QRegExp validatorRegex("[0-9a-zA-Z\\+/]{43,43}=");
> + if (validatorRegex.exactMatch(value))
> + dataMap.insert(QLatin1String(NM_WG_KEY_PRIVATE_KEY), value);
the validation of a key is done multiple times, repeating the same regexp every time -- I'd be worth to create an own validator for it
> wireguard.cpp:159
> + int pos = 0;
> + SimpleIpListValidator *validator = new SimpleIpListValidator(this,
> + SimpleIpListValidator::WithCidr,
"validator" is leaked here -- just use it on the stack (`SimpleIpListValidator validator;`)
> wireguard.cpp:174
> + // Listen Port
> + intValue = interfaceGroup.readEntry(NMV_WG_TAG_LISTEN_PORT, 0);
> + if (intValue > 0) {
`0` is `int` by default, so intValue must be int too (not quint32)
> wireguard.cpp:185
> + if (!value.isEmpty()) {
> + QHostAddress testAddress(value);
> + if (testAddress.protocol() == QAbstractSocket::NetworkLayerProtocol::IPv4Protocol)
const
> wireguard.cpp:195
> + if (intValue > 0) {
> + if (intValue < 65536)
> + dataMap.insert(QLatin1String(NM_WG_KEY_LISTEN_PORT), QString::number(intValue));
an MTU is not a port...
> wireguard.cpp:239
> +{
> + NMStringMap dataMap;
> + NetworkManager::VpnSetting::Ptr vpnSetting = connection->setting(NetworkManager::Setting::Vpn).dynamicCast<NetworkManager::VpnSetting>();
if you move the dataMap declaration after vpnSettings, you can glue declaration and initialization together
> wireguard.cpp:245
> + if (!(dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))
> + || dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4)))
> + || !dataMap.contains(QLatin1String(NM_WG_KEY_PRIVATE_KEY))
most probably this should be `NM_WG_KEY_ADDR_IP6` (not again IP4)
> wireguard.cpp:258-265
> + if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))) {
> + value = dataMap[NM_WG_KEY_ADDR_IP4];
> + if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP6)))
> + value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> + } else if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))) {
> + value = dataMap[NM_WG_KEY_ADDR_IP4];
> + }
just like readEntry, writeEntry can output lists -- just create a QStringList, and pass it to writeEntry
> wireguard.cpp:262-263
> + value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> + } else if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP4))) {
> + value = dataMap[NM_WG_KEY_ADDR_IP4];
> + }
typos for IP6?
> wireguard.ui:54
> + <property name="text">
> + <string>Private Key:</string>
> + </property>
"Private key:"
> wireguard.ui:91
> + <property name="text">
> + <string>Public Key:</string>
> + </property>
"Public key:"
> wireguardadvanced.ui:26
> + <property name="text">
> + <string>Listen Port:</string>
> + </property>
"Listen port:"
> wireguardadvanced.ui:72-76
> + <widget class="QLineEdit" name="fwMarkLineEdit">
> + <property name="toolTip">
> + <string><html><head/><body><p>A 32-bit fwmark for outgoing packets. If set to 0 or "off", this option is disabled. May be specified in hexadecimal by prepending "0x". Optional.</p></body></html></string>
> + </property>
> + </widget>
most probably this should be QSpinBox too...
> wireguardadvanced.ui:103
> + <property name="text">
> + <string>Preshared Key:</string>
> + </property>
"Preshared key:"
> wireguardadvancedwidget.cpp:55
> +
> + QIntValidator *fwMarkValidator = new QIntValidator(0, 4294967295, nullptr);
> + m_ui->fwMarkLineEdit->setValidator(fwMarkValidator);
- 4294967295, other than being a magic value (using std::numeric_limits is a better idea), will even overflow the maximum value for the validator, since it is int (i.e. 32bit signed)
- instead of passing minimum & maximum to constructor, just set the minimum to 0 with `setMinimum`, and do not touch the maximum (which is already the maximum int value)
- `fwMarkValidator` is leaked: use e.g. the lineedit where it will be used as parent
... OTOH, using QSpinBox for this will basically solve all the issues above
> wireguardadvancedwidget.cpp:97
> + else
> + m_ui->presharedKeyLineEdit->setText("");
> +}
`QString()`, instead of `""`
> wireguardadvancedwidget.cpp:115-116
> + intVal = m_ui->listenPortSpinBox->value();
> + stringVal = (intVal > 0) ? QString::number(intVal) : QString("");
> + setOrClear(data, QLatin1String(NM_WG_KEY_LISTEN_PORT), stringVal);
> +
this is basically the positive-or-null int version of setOrClear:
void WireGuardAdvancedWidget::setOrClear(NMStringMap &data, const QLatin1String &key, int value) const
{
if (value > 0)
data.insert(key, QString::number(value));
else
data.remove(key);
}
> wireguardadvancedwidget.cpp:119-120
> + intVal = m_ui->mtuSpinBox->value();
> + stringVal = (intVal > 0) ? QString::number(intVal) : QString("");
> + setOrClear(data, QLatin1String(NM_WG_KEY_MTU), stringVal);
> +
ditto
> wireguardadvancedwidget.h:41-47
> + enum CertCheckType {
> + DontVerify = 0,
> + VerifyWholeSubjectExactly,
> + VerifyNameExactly,
> + VerifyNameByPrefix,
> + VerifySubjectPartially
> + };
is this enum ever used anywhere? if not, please drop
> wireguardadvancedwidget.h:56
> + void loadConfig();
> + void setOrClear(NMStringMap &data, QLatin1String key, QString value) const;
> + Ui::WireGuardAdvancedWidget *m_ui;
const & for both the key and value parameters
> wireguardadvancedwidget.h:57
> + void setOrClear(NMStringMap &data, QLatin1String key, QString value) const;
> + Ui::WireGuardAdvancedWidget *m_ui;
> + class Private;
considering there is a private class already, I'd simply move this private variable to it, with no need to be a pointer variable
> wireguardwidget.cpp:182
> +{
> + return ( (!d->ui.addressIPv4LineEdit->displayText().isEmpty()
> + || !d->ui.addressIPv6LineEdit->displayText().isEmpty())
no need for the (...) for the whole condition:
return foo || bar;
is easier than
return (foo || bar);
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D15093
To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180909/f8da20b0/attachment-0001.html>
More information about the Plasma-devel
mailing list