D15093: Add WireGuard capability.

[Pino Toscano]

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


  In D15093#324457 <https://phabricator.kde.org/D15093#324457>, @andersonbruce wrote:
  
  > A couple of questions on opening other review requests:
  >
  > - Should I open a bug request on bugs.kde.org before opening the review?
  
  
  No need to, they are only internal refactorings/improvements.
  
  > - Is there any special way I need to mark dependencies? That is, the simpleiplistvalidator is dependent on the updates to the simpleipv4/6 validators and WireGuard is dependent on both.
  
  I think so: see on the top-right "Edit Related Revisions" -> "Edit Parent/Child Revisions".
  
  > - Is there a preferred unit test framework that KDE uses?
  
  Qt ships its own QtTest -- see http://doc.qt.io/qt-5/qtest-overview.html
  
  > And do you possibly have a pointer to a sample of other projects that use it?
  
  One simple example is in juk, see the `tests` subdirectory.

INLINE COMMENTS

> simpleiplistvalidator.cpp:27-28
> +SimpleIpListValidator::SimpleIpListValidator(QObject *parent,
> +                                                       enum AddressStyle style,
> +                                                       enum AddressType type)
> +    : QValidator(parent)

this is not C, so 'enum' is not needed

> simpleiplistvalidator.cpp:31-34
> +    addressStyle = style;
> +    addressType = type;
> +    ipv4Validator = nullptr;
> +    ipv6Validator = nullptr;

these can be moved to the constructor initialization list, so it's slightly more efficient

> simpleiplistvalidator.cpp:42
> +            ipv4Style = SimpleIpV4AddressValidator::AddressStyle::WithCidr;
> +        ipv4Validator = new SimpleIpV4AddressValidator(nullptr, ipv4Style);
> +    }

this is leaked -- either you pass 'this' as parent, or explicitly delete them in the class destructor

> simpleiplistvalidator.cpp:50
> +            ipv6Style = SimpleIpV6AddressValidator::AddressStyle::WithCidr;
> +        ipv6Validator = new SimpleIpV6AddressValidator(nullptr, ipv6Style);
> +    }

ditto

> simpleiplistvalidator.cpp:61
> +    // Split the incoming address on commas possibly with spaces on either side
> +    QStringList addressList = address.split(QRegExp("\\s*,\\s*"));
> +

a regexp is not needed here, just pass QString::SkipEmptyParts to split()

> simpleiplistvalidator.cpp:67-68
> +    int i = 0;
> +    QValidator::State ipv4Result = QValidator::Acceptable;
> +    QValidator::State ipv6Result = QValidator::Acceptable;
> +    QValidator::State result = QValidator::Acceptable;

these can be moved inside the foreach loop

> simpleiplistvalidator.cpp:78
> +        if (ipv4Validator != nullptr)
> +            ipv4Result = ipv4Validator->validate(const_cast<QString&>(addr), localPos);
> +        else

this const_cast is not nice... rather keep a local copy of the string

> simpleiplistvalidator.cpp:85
> +        if (ipv6Validator != nullptr)
> +            ipv6Result = ipv6Validator->validate(const_cast<QString&>(addr), localPos);
> +        else

ditto

> simpleiplistvalidator.cpp:91-92
> +        if (ipv6Result == QValidator::Invalid && ipv4Result == QValidator::Invalid) {
> +            result = QValidator::Invalid;
> +            break;
> +        }

i think you can just `return QValidator::Invalid` straight away

> simpleiplistvalidator.h:42-45
> +    AddressStyle addressStyle;
> +    AddressType addressType;
> +    SimpleIpV6AddressValidator *ipv6Validator;
> +    SimpleIpV4AddressValidator *ipv4Validator;

private class variables start with "m_" prefix

> simpleipv4addressvalidator.cpp:26
>  
> -SimpleIpV4AddressValidator::SimpleIpV4AddressValidator(QObject *parent)
> +SimpleIpV4AddressValidator::SimpleIpV4AddressValidator(QObject *parent, enum AddressStyle style)
>      : QValidator(parent)

as above, no "enum"

> simpleipv4addressvalidator.cpp:29
>  {
> +    addressStyle = style;
>  }

in constructor initialization list

> simpleipv4addressvalidator.cpp:55-66
> +        v = new QRegExpValidator(QRegExp(QLatin1String("[0-9, ]{1,3}\\.[0-9, ]{1,3}\\.[0-9, ]{1,3}\\.[0-9, ]{1,3}")), 0);
> +        break;
> +
> +    case WithCidr:
> +        v = new QRegExpValidator(QRegExp(QLatin1String("([0-9, ]{1,3}\\.){3,3}[0-9, ]{1,3}/[0-9]{1,2}")), 0);
> +        break;
> +

all the regexps and their validators are created every time, and this is going to be super slow; since the type is decided at constructor time and never changed, just create the validator once

(alos, this code leaks all the validators)

> simpleipv4addressvalidator.cpp:120
>  
> +
>      // replace input string with the corrected version

unneeded empty line change

> simpleipv4addressvalidator.cpp:137
> +                } else {
> +                    value += QString::number(cidrValue);
> +                    return QValidator::Acceptable;

there is already `cidrParts[1]` as string, so no need to create it again from the value

> simpleipv4addressvalidator.cpp:148
> +                } else {
> +                    value += QString::number(portValue);
> +                    return QValidator::Acceptable;

ditto

> simpleipv4addressvalidator.h:44
> +private:
> +    AddressStyle addressStyle;
>  };

"m_" prefix

> simpleipv6addressvalidator.cpp:26
>  
> -SimpleIpV6AddressValidator::SimpleIpV6AddressValidator(QObject *parent)
> +SimpleIpV6AddressValidator::SimpleIpV6AddressValidator(QObject *parent, enum AddressStyle style)
>      : QValidator(parent)

no need for "enum"

> simpleipv6addressvalidator.cpp:29
>  {
> +    addressStyle = style;
>  }

in constructor initialization list

> simpleipv6addressvalidator.cpp:50-57
> +    case Base:
> +        v = new QRegExpValidator(QRegExp(QLatin1String("([0-9a-fA-F]{1,4}|:)+")), nullptr);
> +        break;
> +
> +    case WithCidr:
> +        v = new QRegExpValidator(QRegExp(QLatin1String("([0-9a-fA-F]{1,4}|:)+/[0-9]{1,3}")), nullptr);
> +        break;

as above, created every time, and leaked

> simpleipv6addressvalidator.h:44
> +private:
> +    AddressStyle addressStyle;
>  };

"m_" prefix

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/20180912/d2011e0a/attachment-0001.html>