Review Request: Proxy overhaul Part 4: More proxy changes and fixes for KProtocolManager
Dawit Alemayehu
adawit at kde.org
Sun Sep 25 17:15:37 BST 2011
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > I'd actually be interested to hear which testing you did.
> >
> > The "ResolveHostNamesBeforeProxyCheck" option seems strange. In which situations is this supposed to be set / not set?
The "ResolveHostNamesBeforeProxyCheck" option is used to give the user the ability to turn DNS lookups of request URLs on or off before checking them against the "No Proxy For" list. This makes it possible for us to let people enter IP address ranges, e.g. "192.168.0.1/24" in the "NoProxyFor" list while at the same time protecting those people that do not want to have any form of name resolution to happen at the client application level. The default behavior is what it currently is, no lookup, since we do not support IP address ranges right now.
As far as testing goes, I created a basic SOCKS server using ssh, ssh -D 9999 127.0.0.1, and a very basic PAC script file that contains the following:
function FindProxyForURL( url, host )
{
var resolved_ip = dnsResolve(host);
if (isInNet(resolved_ip, "127.0.0.1", "255.255.255.0"))
return "DIRECT";
return "SOCKS 127.0.0.1:9999; DIRECT";
}
I am also in the process of testing all these changes agains a real proxy sever. I am going to test against bother privoxy and squid. To test this however, you also need the next patch in the series, https://git.reviewboard.kde.org/r/102696/.
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.h, line 260
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37172#file37172line260>
> >
> > QString::null usage is frowned upon. Just say "empty string".
Ahhh... No one is advocating the use of QString::null. However QString() is a null string, not any empty string. IOW it is not equivalent to QString("") ; so technically saying empty string here is wrong. Null strings are also empty string, but the vise-versa is not true.
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.cpp, line 471
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37173#file37173line471>
> >
> > This looks more like
> > "if no proxy scheme is given, assume SOCKS"
Ahh... Did you mean, "if no proxy could be found for the scheme of the given url, assume SOCKS" ? Even then that comment belongs to the if statement above the one where this comment currently resides. Perhaps I should move the comment down inside the if statement.
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.h, line 278
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37172#file37172line278>
> >
> > What's the use case here? Do you expect ioslaves to try all proxies from the list? Is the list ordered in some way?
> >
> > How is the list of proxy server addresses related to @p url? It's obvious from the name, but it is good practice to state all the important facts in the apidox.
Proxy Auto Configuration script files (aka PAC files) can return multiple proxy addresses, e.g. "SOCKS 127.0.0.1:8080; PROXY 127.0.0.1:3128; DIRECT" [1]. The intent of that is to have some type of fault tolerance where if connecting to one proxy server fails, you should use the next one in the order presented. We never supported this in KDE because no one bothered or cared enough to do it before now. The choice to support this more toerant implementation is up to each ioslave and hence the patch for kio_http.
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.h, line 257
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37172#file37172line257>
> >
> > This is not a method that allocates a resource, for which "request" is the right word. It's a method that answers a question.
> > I would suggest something like "Returns the proxy server to use to contact @p url, or "DIRECT" if no proxy should be used."
> >
> > I don't understand the part about the empty string at all. How is it different from "DIRECT"?
It is not. Both proxyFor and proxyForUrl will always "DIRECT" if no proxy is found for the given url. The documentation needs to be updated.
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.h, line 271
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37172#file37172line271>
> >
> > See above. Client programmers don't request a proxy (the meaning of that is closer to "create me one"), they ask for the address of an existing one.
Right. The list will never be empty. It will at least contain "DIRECT".
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.h, line 234
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37172#file37172line234>
> >
> > What is a partial host name? I know that the wording isn't your fault, but maybe you know what it means.
I just gone ahead and completely changed the entire documentation for that function. It should make better sense now.
- Dawit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102691/#review6796
-----------------------------------------------------------
On Sept. 25, 2011, 4:15 p.m., Dawit Alemayehu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102691/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2011, 4:15 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> -------
>
> This patch is the 4th in the serious of patches designed to resolve bugs and missing functionality in KDE's proxy manager. The changes made with this patch are as follows:
>
> * Add code that resolves a request url's hostname before attempting to match
> it against the no proxy for list so long as the "ResolveHostNamesBeforeProxyCheck"
> option is set.
>
> * Allow "DIRECT" as a special keyword in the list of proxy server addresses
> returned in slaveProtocol(const QString& protocol, QStringList& proxy).
>
> * Change KProtocolManager::proxyFor to properly handle the changes in the new
> proxy management dialog (KDE 4.8) where the proxy server port, in the
> manual proxy configuration mode, will be saved separated from the address with
> a white space.
>
> * Move the code that accounts for SOCKS proxy from KProtocolManager::proxyFor
> to KProtocolManager::proxyForUrl where it belongs. The current implementation
> only works correctly under one circumstance while breaking the previous behavior
> of the function.
>
> * Fix KProtocoManager::proxiesForUrl so that it accounts for the proxy
> exception list.
>
> * Update API documentation to reflect the changes above.
>
>
> Diffs
> -----
>
> kio/kio/kprotocolmanager.h 11e43fe
> kio/kio/kprotocolmanager.cpp 50ebb6e
>
> Diff: http://git.reviewboard.kde.org/r/102691/diff/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dawit Alemayehu
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110925/6a23e256/attachment.htm>
More information about the kde-core-devel
mailing list