Differential D25432

Fix HTTP proxy settings
ClosedPublic
Actions

Authored by trufanov on Nov 20 2019, 10:41 PM.

Details

Reviewers
meven
Group Reviewers
Frameworks
Commits
R241:c0ae03d14b0e: Fix HTTP proxy settings
Summary

After latest libKF5KIOCore.so.5.65.0 update my KTorrent has stopped to work via HTTP Proxy.
I've created a Bug here: https://bugs.kde.org/show_bug.cgi?id=414346 to make it more googleable if someone else faces with this problem now.
The bug in the line here: https://github.com/KDE/kio/commit/0911763b8f19c9fee3083e7f09382e1cde957723#diff-7d1e19d94808019256c1784b3dfc6801R2163
The method returns QVariant with type QByteArray that can't be directly converted to QStringList. It should be converted to QString and splitted.

BUG: 414346
FIXED-IN: 5.65

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trufanov created this revision.Nov 20 2019, 10:41 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 20 2019, 10:41 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
trufanov requested review of this revision.Nov 20 2019, 10:41 PM
meven edited the summary of this revision. (Show Details)Nov 21 2019, 8:59 AM
meven added a reviewer: Frameworks.
meven accepted this revision.Nov 21 2019, 9:06 AM

Thank you for digging it !

KConfig did quite some work KConfigGroupPrivate::deserializeList
https://cgit.kde.org/kconfig.git/tree/src/core/kconfiggroup.cpp#n155

But a simple toString().split() is seems correct to me.

This revision is now accepted and ready to land.Nov 21 2019, 9:06 AM
Closed by commit R241:c0ae03d14b0e: Fix HTTP proxy settings (authored by trufanov). · Explain WhyNov 21 2019, 11:39 AM
This revision was automatically updated to reflect the committed changes.
trufanov added a comment.Nov 21 2019, 1:39 PM

Hmm, I've just git grep the repo and found another place that is most probably affected by the same problem in FTP slave;
https://github.com/KDE/kio/blob/d03551cd0367ac7760224c56bda943c937ba5dac/src/ioslaves/ftp/ftp.cpp#L320

Could you check out if it's a QVariant with QByteArray type and fix it on your own?

Also now I think the code would be more self-descriptionly if I would use QByteArray() instead of QString() as a default value for mapConfig().value() in my fix.
So if the second place is a bug and you think the same about default values feel free to improve my commit in HTTP with your fix too.

meven added a comment.Nov 21 2019, 2:27 PM
In D25432#565595, @trufanov wrote:

Hmm, I've just git grep the repo and found another place that is most probably affected by the same problem in FTP slave;
https://github.com/KDE/kio/blob/d03551cd0367ac7760224c56bda943c937ba5dac/src/ioslaves/ftp/ftp.cpp#L320

Could you check out if it's a QVariant with QByteArray type and fix it on your own?

Also now I think the code would be more self-descriptionly if I would use QByteArray() instead of QString() as a default value for mapConfig().value() in my fix.
So if the second place is a bug and you think the same about default values feel free to improve my commit in HTTP with your fix too.

Good point this needs to be fixed as well since mapConfig QVariant will always be a QByteArray QVariant : https://github.com/KDE/kio/blob/f2a3a78972b2674269b2179a8866ac5480f7c51e/src/core/slavebase.cpp#L1199

dfaure added a subscriber: dfaure.Nov 23 2019, 7:10 PM

This doesn't appear to fix the bug for me. I get a stringlist with one empty string in it, which goes to the "else" branch on line 2712.
This is very probably the reason for the ftp and http tests being broken in CI too.

I'll a SkipEmptyParts to the split(), but this is getting a bit hairy...

dfaure added a comment.Nov 23 2019, 7:13 PM

Done in https://commits.kde.org/kio/48b8cab8da5e264f233d59cf2aff2b981255f6bf

Please check if other places are affected.

This code could be simplified by using configValue() instead, no? Right now the QString() argument serves no purpose, it gets converted to QVariant.
This would allow removing the toString() call.

Revision Contents
Changeset List

PathPackages
Msrc/ioslaves/http/http.cpp (2 lines)

Diff 70098

View Options

src/ioslaves/http/http.cpp

Loading...