add initial wsdiscovery support
AcceptedPublic

Authored by sitter on Dec 2 2019, 1:50 PM.

Details

Reviewers
dfaure
ngraham
Group Reviewers
Frameworks
Dolphin
Summary

needs kdsoap >= 1.8.50 (current master).
builds a static variant of kdsoap-ws-discovery-client but can also
use system's version if available.

also reinvents how discovery works:
there are now Discoverers for dnssd and wsd that get started/stopped.
the Discoverers discover servers and emit them as Discoveries. Discoveries
are then converted to udsentries for kio.

BUG: 392447
FIXED-IN: 20.04.0

Test Plan

discover all the things!

Diff Detail

Repository
R320 KIO Extras
Branch
arcpatch-D25682
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21232
Build 21250: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

Why do we need to mirror this dsoap-ws-discovery-client lib that seems to be copied from somewhere?

Testing convenience until @caspermeijn makes a release mostly. The library was,or maybe even still is, in flux as API was being shuffled up to kdsoap.

ngraham added a subscriber: ngraham.Dec 2 2019, 3:22 PM

OMG it works!!! I can find my windows Samba share in Dolphin again!

Why do we need to mirror this dsoap-ws-discovery-client lib that seems to be copied from somewhere?

Testing convenience until @caspermeijn makes a release mostly. The library was,or maybe even still is, in flux as API was being shuffled up to kdsoap.

Cool than my library can be used for this as well! The interface to KDSoap is in master now, so that only waits for a release of KDSoap. However I want to change some things in the kdsoap-ws-discovery-client API.

Please be aware of the license of the WSDL files. It explicitly states "this document itself may not be modified in any way", so I am not sure what this will to with the license of the binary.

dfaure requested changes to this revision.Dec 7 2019, 12:27 AM

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 :-)

smb/discovery.cpp
22

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.

smb/discovery.h
3

What happened to the '>' character?

[repeats]

41

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.

52

const?

smb/dnssddiscoverer.cpp
47

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

72

qAsConst

104

qAsConst

smb/dnssddiscoverer.h
42

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)

smb/kdsoap-ws-discovery-client/.gitlab-ci.yml
3

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

smb/kdsoap-ws-discovery-client/CMakeLists.txt.user
3 ↗(On Diff #70750)

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

205 ↗(On Diff #70750)

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

smb/kdsoap-ws-discovery-client/src/wsdiscoverytargetservice.cpp
39

const

(this will avoid detaching m_typeList)

49

same

smb/kio_smb_browse.cpp
478

lol

493

isEmpty() is more readable.

502

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

list.append(discovery->toEntry())

(another move, no copy)

510

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

518

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++.

smb/wsdiscoverer.cpp
117

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

129

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

161

unused

210

qAsConst

auto & ?

253

then rewrite this code to .at(0) ?

258

especially with the data races.

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

281

Obviously unfinished :-)

smb/wsdiscoverer.h
49

(same question about virtual)

This revision now requires changes to proceed.Dec 7 2019, 12:27 AM

With regards to the Docker/Gitlab CI part, please use the images under kdeorg/ on Dockerhub rather than personally maintained images as the wider community has no access to your namespace on Gitlab.com

(Additionally please note that Fedora is not permitted to be used on our infrastructure)

sitter marked 26 inline comments as done.Dec 9 2019, 3:34 PM

Yep, still looking for primarily design input.

Thanks for your comments, David!

smb/discovery.h
41

Good point. We used to pass it around before, but it indeed doesn't make much sense.

smb/dnssddiscoverer.h
42

Left over from something I tried, not actually necessary indeed.

smb/kdsoap-ws-discovery-client/.gitlab-ci.yml
3

No. Well, I mean, it's a copy of the upstream repo with only the actually necessary delta applied (STATIC internal build as opposed to SHARED).

smb/kdsoap-ws-discovery-client/CMakeLists.txt.user
3 ↗(On Diff #70750)

Yeah. Slipped into git add.

smb/kdsoap-ws-discovery-client/src/wsdiscoverytargetservice.cpp
39

Forwarded to upstream repo.

smb/wsdiscoverer.cpp
210

waitForFinished is not const. I've made the container qAsConst though.

253

That's the plan, I need to read up on when exactly there'd be more than one xaddr though. While we need only one I haven't read up if any xaddr will work all the time.

258

promoted the comment to a warning for now so I don't forget to deal with it before landing

281

Yep. Only there to easily see which service produced which device entry for debugging ;)

sitter updated this revision to Diff 71121.Dec 9 2019, 3:35 PM
sitter marked 9 inline comments as done.

various fixes suggested by dfaure

dfaure requested changes to this revision.Dec 13 2019, 9:07 PM
dfaure added inline comments.
smb/discovery.cpp
32

Lines 30-31 duplicate lines 23-28, surely this can't link without errors?

smb/dnssddiscoverer.cpp
30

All toEntry() methods should be const, no?

49

smb, not smb://

75

"it" is a confusing name for the reader, but it's not an iterator. It's a service pointer.
Maybe call it servicePtr?

Then *servicePtr will make it clear that you're comparing services instances.

smb/wsdiscoverer.cpp
129

When the local variable is const, you don't need qAsConst on top :-)

288

"smb"

This revision now requires changes to proceed.Dec 13 2019, 9:07 PM
meven added a subscriber: meven.Jan 6 2020, 9:19 AM

A few qDebug not to forget to clean before merging.

smb/kio_smb_browse.cpp
375

remove

538

qCDebug or remove

544

qCDebug or remove

smb/wsdiscoverer.cpp
89

Remove

117

qCDebug or remove
qCWarning

153

qCDebug or remove

156

qCDebug or remove

165

remove

210

qCDebug or remove

225

qCDebug or remove

241

qCDebug or remove

263

qCDebug or remove

268

qCDebug or remove

sitter updated this revision to Diff 73405.Jan 13 2020, 1:11 PM
sitter marked 19 inline comments as done.

since there's not been much design input let's move to actual review

primary change here is that wsdiscoverer no longer uses qtconcurrent but
instead signal slots on the same thread. ultimately discovery is blocking
listDir, so the threading offerend very little advantage.
I am not quite sure how to best implement the previous stop behavior where
I'd wait on the qfuture. I've replaced it with a qapp::processevents, but
I'm not sure that is the best approach.

listDir itself still would benefit from a larger refactor to split it
into its parts, that's not really impacting the change under review here
though.

sitter retitled this revision from [WIP] add initial wsdiscovery support to add initial wsdiscovery support.Jan 13 2020, 1:12 PM
sitter edited the summary of this revision. (Show Details)
sitter added inline comments.
smb/discovery.cpp
32

They are not. Discovery vs. Discoverer. I am not super fond of the naming though, for exactly that reason :S

smb/wsdiscoverer.cpp
129

Yes.. But... I do remember reviews where people asked for qAsConst on top of const even after it was pointed out that it doesn't do anything. I thought that it is kinda nit picky but at the same time for code longevity it probably makes more sense to ignore contextual const, after all, should the local need to be made !const one couldn't forget to update the loop. But then I suppose clazy would lament anyway. ¯\_(ツ)_/¯ best sums up all my feelings on everything to do with qAsConst TBH

dfaure added inline comments.Jan 15 2020, 9:41 PM
smb/wsdiscoverer.cpp
98

You could also connect the signal resolved() to a local QEventLoop here? Would be cleaner and less CPU-intensive.

sitter updated this revision to Diff 73704.Jan 16 2020, 2:06 PM

redesign finish system

it occured to me that the blocking nature of the finish system is entirely pointless.
we already have an eventloop running anyway, so all we need to do is track whether all
entities have been resolved to discoveries. when they have the discoverer is finished.
when all discoverers are finished the entire loop and thus the browse cmd can terminate.

also fixed some problems with forced eventloop quitting when the hard timeout is hit.
before quitting the eventloop the udsentry list is flushed now, otherwise we may
lose discoveries depending on timing

ngraham accepted this revision as: ngraham.Fri, Jan 31, 1:22 PM

Works great, super cool to have this.

Since kdsoap-ws-discovery-client is upstream now, do we still need to duplicate it here?

smb/kdsoap-ws-discovery-client/.gitignore
1

shouldn't you be doing out-of-source builds lol

sitter added inline comments.Fri, Jan 31, 3:03 PM
smb/kdsoap-ws-discovery-client/.gitignore
1

an unrelated subdir is still technically out-of-source from a cmake pov ;)

Any further comments? @dfaure in particular.

dfaure accepted this revision.Mon, Feb 3, 9:48 PM
This revision is now accepted and ready to land.Mon, Feb 3, 9:48 PM
sitter added a subscriber: winterz.Fri, Feb 7, 12:45 PM

Oh dear! I just noticed that kdsoap 1.9 isn't out yet. @dfaure @winterz any chance of a release some time soon?

Yay. 1.9 is out.
I've poked our CI maintainers about including kdsoap 1.9 and have high hopes at least suse is done by Monday. I am kinda leaning towards landing the diff on Monday regardless as I'd rather have this get more exposure sooner rather than later.