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 &quot;off&quot;, this > option is disabled. May be specified in hexadecimal by prepending > &quot;0x&quot;. 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
