D15093: Add WireGuard capability.

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


  Much better now!
  
  General notes:
  
  - there are various checks on lengths of strings like `str.length() > 0` or `str.length() != 0`: if all you need is check whether a string is empty or not, just use `str.isEmpty()`
  - regarding the UI for all the pre/post scripts: since they are file paths, better use a KUrlRequester widget (limited to local existing files only, no URLs), so the users have a Browse button next to each line edit that can be used to open a file dialog

INLINE COMMENTS

> nm-wireguard-service.h:2
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
> +/* nm-openvpn-service - openvpn integration with NetworkManager
> + *

this comment needs to be fixed

> wireguard.cpp:25
> +#include <QStringBuilder>
> +#include <QFile>
> +#include <QFileInfo>

unused

> wireguard.cpp:29-30
> +#include <KLocalizedString>
> +#include <KMessageBox>
> +#include <KStandardDirs>
> +#include <KConfig>

both unused

> wireguard.cpp:41
> +
> +#include <arpa/inet.h>
> +

unused

> wireguard.cpp:60
> +#define NMV_WG_TAG_FWMARK                "FwMark"
> +#define NMV_WG_ASSIGN                    "="
> +

unused

> wireguard.cpp:151-153
> +    KConfig importFile(fileName, KConfig::NoGlobals);
> +    KConfigGroup interfaceGroup = importFile.group(NMV_WG_TAG_INTERFACE);
> +    KConfigGroup peerGroup = importFile.group(NMV_WG_TAG_PEER);;

make all 3 as `const`, so it is clear they are read-only (and attempts to use writeEntry() will result in build failures)

> wireguard.cpp:153
> +    KConfigGroup interfaceGroup = importFile.group(NMV_WG_TAG_INTERFACE);
> +    KConfigGroup peerGroup = importFile.group(NMV_WG_TAG_PEER);;
> +

extra ';'

> wireguard.cpp:166-172
> +    // The config file must have both [Interface] and [Peer] sections
> +    if (!importFile.groupList().contains("Interface")
> +        || !importFile.groupList().contains("Peer")) {
> +        mError = VpnUiPlugin::Error;
> +        mErrorMessage = i18n("Could not open file");
> +        return result;
> +    }

These checks can be moved right after reading the config file and getting the KConfigGroup objects.

> wireguard.cpp:167-168
> +    // The config file must have both [Interface] and [Peer] sections
> +    if (!importFile.groupList().contains("Interface")
> +        || !importFile.groupList().contains("Peer")) {
> +        mError = VpnUiPlugin::Error;

You do not need to get the list of groups in the config file (twice): earlier you get the KConfigGroup objects for the groups, so checking eg `interfaceGroup.exists()` should do the job.

> wireguard.cpp:175-178
> +    value = interfaceGroup.readEntry(NMV_WG_TAG_ADDRESS);
> +    {
> +        QStringList addressList;
> +        addressList << value.split(QRegExp("\\s*,\\s*"));

KConfig already supports comma-separated lists -- just pass QStringList() as `default` value to readEntry(), so KConfigGroup knows the value is a list.

> wireguard.cpp:195
> +    // Listen Port
> +    value = interfaceGroup.readEntry(NMV_WG_TAG_LISTEN_PORT);
> +    if (value.length() > 0) {

If you specify `0` as default parameter, KConfigGroup will try to decode the value as integer automatically. If `0` is a valid value for the port, then use `-1`.
After doing that, you do not need a validator anymore, just a manual range check will do the job.

> wireguard.cpp:209
> +        QRegExp validatorRegex(*regexStrings.keySpec);
> +        QRegExpValidator validator(validatorRegex);
> +        int pos = 0;

this validator is not needed, just use `validatorRegex` directly

> wireguard.cpp:231
> +    // MTU
> +    value = interfaceGroup.readEntry(NMV_WG_TAG_MTU);
> +    if (value.length() > 0) {

same as for the listen port: please read the value directly as integer.

> wireguard.cpp:271
> +        QRegExp validatorRegex(*regexStrings.keySpec);
> +        QRegExpValidator validator(validatorRegex);
> +        int pos = 0;

as above, no need for a validator, just use the QRegExp directly

> wireguard.cpp:288
> +                                 + ", *)*" + *regexStrings.ip4Orip6Address);
> +        QRegExpValidator *validator = new QRegExpValidator(allowedIPsRegex, this);
> +        if (validator->validate(value, pos) != QValidator::State::Invalid) {

as above, no need for a validator, just use the QRegExp directly

> wireguard.cpp:306
> +        QRegExp validatorRegex(*regexStrings.keySpec);
> +        QRegExpValidator validator(validatorRegex);
> +        int pos = 0;

as above, no need for a validator, just use the QRegExp directly

> wireguard.cpp:350-353
> +        value = dataMap[NM_WG_KEY_ADDR_IP4];
> +        if (dataMap.contains(QLatin1String(NM_WG_KEY_ADDR_IP6))) {
> +            value += "," + dataMap[NM_WG_KEY_ADDR_IP6];
> +        }

as mentioned above: KConfigGroup supports lists

> wireguard.cpp:361-365
> +    // Do Private Key
> +    if (dataMap.contains(QLatin1String(NM_WG_KEY_PRIVATE_KEY)))
> +        interfaceGroup.writeEntry(NMV_WG_TAG_PRIVATE_KEY, dataMap[NM_WG_KEY_PRIVATE_KEY]);
> +    else
> +        return false;

this, just like all the other required keys, should be checked before even opening the output file

> wireguard.ui:23-25
> +      <property name="fieldGrowthPolicy">
> +       <enum>QFormLayout::ExpandingFieldsGrow</enum>
> +      </property>

please leave the default behaviour, which is set by the style and/or the platform plugin

> wireguard.ui:26-28
> +      <property name="verticalSpacing">
> +       <number>6</number>
> +      </property>

no hardcoded margins/spacings please

> wireguard.ui:39
> +        <property name="toolTip">
> +         <string><html><head/><body><p>IPv4 intrnrt address assigned to the local interface. IPv4 or IPv6 address (or both) required</p></body></html></string>
> +        </property>

typo "Internet" (proper noun), also in other places in this .ui file

> wireguardadvanced.ui:39
> +        <property name="toolTip">
> +         <string><html><head/><body><p>A 16-bit port for listening. Optional; if not specified, chosen randomly.</p></body></html></string>
> +        </property>

IMHO specifying that is "16-bit" is out of scope, and of no value

> wireguardadvanced.ui:51-55
> +       <widget class="QLineEdit" name="mTULineEdit">
> +        <property name="toolTip">
> +         <string><html><head/><body><p>If not specified, the MTU is automatically determined from the endpoint addresses or the system default route, which is usually a sane choice. However, to manually specify an MTU to override this automatic discovery, this value may be specified explicitly.</p></body></html></string>
> +        </property>
> +       </widget>

IMHO this is better as QSpinBox, with `0` as default value (see also the `specialTextValue` property of QAbstractSpinBox).
This is also what the MTU configuration for wired connections uses, btw.

> wireguardadvancedwidget.h:59
> +    void loadConfig();
> +    void setOrClear(NMStringMap &data, QLatin1String key, QString value) const;
> +    Ui::WireGuardAdvancedWidget *m_ui;

const & for both the key and value parameters

> wireguardauth.cpp:49-50
> +
> +
> +
> +QVariantMap WireGuardAuthWidget::setting() const

extra empty lines

> wireguardwidget.cpp:31-32
> +
> +#include <KProcess>
> +#include <KUrlRequester>
> +

undeeded

> wireguardwidget.h:26
> +
> +#include <QProcess>
> +

unneeded

> wireguardwidget.h:58
> +    Private *d;
> +    void setOrClear(NMStringMap &data, QLatin1String key, QString value) const;
> +

const & for both the key and value parameters

REPOSITORY
  R116 Plasma Network Management Applet

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

To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, K900, anthonyfieroni, 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/20180903/aef7cc45/attachment-0001.html>