D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[David Faure]

dfaure added a comment.


  Why does KRun duplicate all of the (new) code from DesktopExecParser, when DesktopExecParser is actually a helper class for KRun?
  I would expect it to have solved all this already, unless I'm missing something about the various code paths.
  The code you changed in krun.cpp was supposed to simply resolve desktop:/foo to file:///bleh but everything else about %u/%f is done by DesktopExecParser.

INLINE COMMENTS

> desktopexecparser.cpp:311
>  
>      // Check if we need kioexec
>      bool useKioexec = false;

`, or KIOFuse`

> desktopexecparser.cpp:317
> +    struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; };
> +    QVector<MountRequest> requests;
>      if (!mx1.hasUrls) {

`requests.reserve(d->urls.count());`

> desktopexecparser.cpp:319
>      if (!mx1.hasUrls) {
> -        for (const QUrl &url : qAsConst(d->urls)) {
> +        for (int i = 0; i < d->urls.length(); ++i)  {
> +            const QUrl url = d->urls[i];

Minor: can you use count() (Qt-like) or size() (STL-like)? In 20 years I never saw code that uses length() for a vector or a list, it's just very unusual in Qt code :-)

[occurs twice]

> desktopexecparser.cpp:320
> +        for (int i = 0; i < d->urls.length(); ++i)  {
> +            const QUrl url = d->urls[i];
>              if (!url.isLocalFile() && !hasSchemeHandler(url)) {

Use d->urls.at(i) to avoid a detach given that d->urls isn't const here.

[occurs twice]

> desktopexecparser.cpp:323
> +                // Lets try a KIOFuse mount instead.
> +                requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
>              }

Could we have a kill switch for KIOFuse if it fails to work properly in some release?

E.g. some `export KIO_FUSE=0` or `export KIO_FUSE_DISABLE=1`.
Or a KConfig entry (use `KSharedConfig::openConfig()->group("KIO")` for best performance and flexibility -- then it can be disabled for one calling application or for all)

> desktopexecparser.cpp:330
> +            // NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://)
> +            // but will struggle with passwords.
> +            // Hence convert URL to KIOFuse equivalent in case there is a password.

"Struggle" sounds like the apps are limited/buggy.

But if what you mean is that the password (e.g. stored in kpasswdserver after the user typed it in the KDE dialog asking for it) won't be available to non-KIO applications, that seems logical and I do accept that argument. It's just the writing that is surprising.

> desktopexecparser.cpp:331
> +            // but will struggle with passwords.
> +            // Hence convert URL to KIOFuse equivalent in case there is a password.
> +            // @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/

The comment says "in case there is a password", the code doesn't.
Probably historical, please fix :)

On this topic I saw earlier comments in the merge request which I found confusing.
It's very rare for an application to have the password stored inside the URL itself. That would mean the user typing it in clear in a location bar or in a terminal, bad idea. Instead, the user typically types a URL like ftp://user@host/ and the kioslave asks for the password, and stores it in kpasswdserver.

Also you wrote "we strip out the stuff that's in userInfo()". (where QUrl::userInfo is username+password). But surely, while we might strip out passwords for security reasons, we never strip out the username, do we?

> desktopexecparser.cpp:348
> +                // Lets try a KIOFuse mount instead.
> +                requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
>              }

This duplicates line 344.

`if (a) { if (!b) { foo } } else { foo }`
is equivalent to
`if (!a || !b) { foo }`

In your case `a` is `http || https` which makes this a big uglier, but you could use what other pieces of code do and write url.scheme().startsWith(QLatin1String("http")).

So this becomes `if (!url.scheme().startsWith(QLatin1String("http")) || !supportedProtocols(d->service).contains(url.scheme())) {`

> desktopexecparser.cpp:355
> +    // Main thing that we want is to send the mount requests without blocking.
> +    for (auto request : requests) {
> +        request.reply.waitForFinished();

`auto &request` to avoid copying

> desktopexecparser.cpp:356
> +    for (auto request : requests) {
> +        request.reply.waitForFinished();
> +        if (request.reply.isError()) {

This blocks.

I don't mind much myself, but I know some people had a mandate to remove as many blocking calls as possible from KRun and related code. Or was that only avoiding blocking I/O because of network mounts? [which this patch is all about adding more of...]. @broulik ?

> desktopexecparser.cpp:378
>          return result;
> +    } else {
> +        // At this point we know we're not using kioexec, so feel free to replace

This nesting under `else` is unnecessary given the `return` just above. It's even a bit confusing that some code is in `else` and then some code is outside the `if/else`, but the '}' could be anywhere in between, that would make no difference.

I would suggest to just remove the `else`.

> desktopexecparser.cpp:381
> +        // KIO URLs with their KIOFuse local path.
> +        for (auto request : requests) {
> +            d->urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value());

`for (const auto & request : qAsConst(requests))`

or use std::vector if you find qAsConst ugly :-)

> CMakeLists.txt:80
>  qt5_add_dbus_interface(kiowidgets_SRCS org.kde.kuiserver.xml kuiserver_interface)
> +qt5_add_dbus_interface(kiowidgets_SRCS org.kde.KIOFuse.VFS.xml kiofuse_interface)
>  

Just use ../core/org.kde.KIOFuse.VFS.xml instead of duplicating the file.

But even better would be to not need it in KRun, see separate comment.

> krun.cpp:581
> +        QVector<MountRequest> requests;
> +        for (int i = 0; i < urls.length(); ++i) {
> +            const QUrl url = urls[i];

count or size

> krun.cpp:582
> +        for (int i = 0; i < urls.length(); ++i) {
> +            const QUrl url = urls[i];
> +            // NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://)

.at(i)

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191204/bf16029f/attachment-0001.html>