D25682: [WIP] add initial wsdiscovery support

[David Faure]

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


  I guess you were expecting a higher-level review, but I don't know anything about these protocols.
  
  I'm glad to see my KDSoap library being useful in KDE too though :-)

INLINE COMMENTS

> discovery.cpp:21
> +
> +#include "discovery.h"

Not sure this .cpp file serves any purpose right now ;)

But actually the virtual destructors could be implemented here out-of-line to avoid being generated in every user of the class. The `=default` syntax would work here too.

> discovery.h:2
> +/*
> +    Copyright 2019 Harald Sitter <sitter at kde.org
> +

What happened to the '>' character?

[repeats]

> discovery.h:40
> +    virtual ~Discovery() = default;
> +    virtual void toEntry(KIO::UDSEntry &entry) = 0;
> +};

Why don't these `toEntry()` methods *return* a KIO::UDSEntry instead?
It's always empty on incoming anyway.

And UDSEntry supports moving so the "return" statement won't make a copy.

> discovery.h:51
> +    virtual void stop() = 0;
> +    virtual bool isFinished() = 0;
> +

const?

> dnssddiscoverer.cpp:46
> +    //   that assumption will be true all the time. ~sitter, 2018
> +    QUrl u(QStringLiteral("smb://"));
> +    u.setHost(m_service->hostName());

`u.setScheme(QStringLiteral("smb"))` would be slightly faster; you're constructing a URL from its parts, no need to trigger parsing.

> dnssddiscoverer.cpp:71
> +        // RemoteService::Ptr is useless here.
> +        for (const auto &it : m_services) {
> +            if (*service == *it) {

qAsConst

> dnssddiscoverer.cpp:103
> +    m_browser.disconnect();
> +    for (auto service : m_services) {
> +        service->resolve(); // Blocks until resolution happened. Our signal handle then jumps in.

qAsConst

> dnssddiscoverer.h:41
> +
> +class DNSSDDiscoverer : public QObject, public virtual Discoverer
> +{

Why `virtual`? I thought this only mattered for diamond-shaped inheritance.

(OTOH I remember it leads to strange order of ctors being called, so I avoid it as much as possible)

> .gitlab-ci.yml:2
> +fedora:
> +    image: registry.gitlab.com/caspermeijn/docker-images/fedora-build-onvifviewer:latest
> +    stage: build

(is this meant to go into kde git? just wondering)

> CMakeLists.txt.user:3
> +<!DOCTYPE QtCreatorProject>
> +<!-- Written by QtCreator 4.9.1, 2019-08-07T17:08:05. -->
> +<qtcreator>

Now this one I'm sure, should NOT go into git.

> CMakeLists.txt.user:205
> +    </valuelist>
> +    <value type="QString" key="ProjectExplorer.BuildConfiguration.BuildDirectory">/home/me/src/git/soapy/build-kdsoap-ws-discovery-client-Desktop-Release-with-Debug-Information</value>
> +    <valuemap type="QVariantMap" key="ProjectExplorer.BuildConfiguration.BuildStepList.0">

... because it's about your config :)

> wsdiscoverytargetservice.cpp:38
> +
> +bool WSDiscoveryTargetService::isMatchingType(const KDQName &matchingType)
> +{

const

(this will avoid detaching m_typeList)

> wsdiscoverytargetservice.cpp:48
> +
> +bool WSDiscoveryTargetService::isMatchingScope(const QUrl &matchingScope)
> +{

same

> kio_smb_browse.cpp:479
> +       if (normalizedUrl.path().isEmpty()) {
> +#pragma message "refactor this entire function into less of a long pile of madness"
> +           qDebug() << "Trying modern discovery (dnssd/wsdiscovery)";

lol

> kio_smb_browse.cpp:494
> +           connect(&sendTimer, &QTimer::timeout, this, [&] {
> +               if (list.size() < 1) {
> +                   return;

isEmpty() is more readable.

> kio_smb_browse.cpp:503
> +           auto enterDiscovery = [&](Discovery::Ptr discovery) {
> +               discovery->toEntry(udsentry);
> +               list.append(udsentry);

With my suggestion to return a udsentry, this whole lambda becomes

`list.append(discovery->toEntry())`

(another move, no copy)

> kio_smb_browse.cpp:511
> +
> +           QList<Discoverer *> discoverers {&d, &w};
> +

const QList.... to avoid detaching in range-for below

> kio_smb_browse.cpp:519
> +                   }
> +                   allFinished &= discoverer->isFinished();
> +               }

never use &= for booleans, it's not meant for that

(it won't work for '1' and '2' which are both 'true' for booleans)

`allFinished = allFinished && discoverer->isFinished()`

Unfortunately there's no &&= in C++.

> wsdiscoverer.cpp:116
> +            qWarning() << response.faultAsString();
> +            // No return! We'd disqualify systems that do not implement pbsd.
> +        }

OK. But maybe an `else` then ?
Not much point in using response.childValues() on a fault, i.e. iterating over the fault details.

> wsdiscoverer.cpp:128
> +        QString computer;
> +        for (auto section : response.childValues()) {
> +            computer = section

Needs a const local var to avoid a detach. Yes, range-for is annoying for Qt containers.

> wsdiscoverer.cpp:160
> +
> +        const QHostAddress address(m_endpointUrl.toString());
> +        qDebug() << "++++++++++++++++++++++++++++++++++++++++++++";

unused

> wsdiscoverer.cpp:209
> +    // We do set a suitable timeout in the resolver so this doesn't take forever.
> +    for (auto future : m_futures) {
> +        future.waitForFinished();

qAsConst

auto & ?

> wsdiscoverer.cpp:252
> +        addr = xAddr;
> +#warning only get first val we only need one addr
> +    }

then rewrite this code to `.at(0)` ?

> wsdiscoverer.cpp:257
> +
> +    // Probably should just qobject this. Hardly worth the threading.
> +    m_futures << QtConcurrent::run([=] {

especially with the data races.

(service->endpointReference() reads data written by another thread, without mutex)

> wsdiscoverer.cpp:280
> +    entry.fastInsert(KIO::UDSEntry::UDS_ICON_NAME, "network-server");
> +    entry.fastInsert(KIO::UDSEntry::UDS_ICON_OVERLAY_NAMES, "/home/me/.local/share/icons/Windows10Icons/32x32/places/ubuntu-logo.png");
> +

Obviously unfinished :-)

> wsdiscoverer.h:48
> +
> +class WSDiscoverer : public QObject, public virtual Discoverer
> +{

(same question about virtual)

REPOSITORY
  R320 KIO Extras

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

To: sitter, dfaure, #frameworks, #dolphin
Cc: ngraham, caspermeijn, davidedmundson, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20191207/cf373523/attachment.htm>