[PATCH] DNS cache for Konqueror/KIO

[Thiago Macieira]

Roland Harnau wrote:
>2008/6/23, Thiago Macieira <thiago at kde.org>:
>> I didn't like the indention style. Please don't put function opening
>> brackets on the same line as their declaration. You also have
>> whitespace errors (lines ending in whitespace).
>
>The location of the opening brackets is maybe too much of Java style
>for KDE. Attached is somewhat improved patch adhering a bit more to
>kdelibs coding style and  hopefully without superfluous whitespace.

Ok, I have finally got my act together and reviewed the patch.

You made the file a .h (hostinfoagent.h). The class is not public, though. 
Can you make it a private header instead (hostinfoagent_p.h)?

Why do we need a static function to return the instance to call 
lookupHost()? Can't lookupHost() be static instead?

Please, no private static variables (HostInfoAgent::m_instance). Move the 
variable to file-static please.

I also don't like the operator<< and operator>> you added. Those become 
global in any KDE application linking to kio. Please remove them and 
marshall/demarshall the data manually. (i.e., move the code from those 
operators into the functions that are calling them now, that is, 
KIO::SlaveBase::waitForHostInfo and KIO::SlaveInterface::setHostInfo 
[btw, this last one has a bad name for a slot])

KIO::SlaveInterface is a public class, so please no private members. Use 
Q_PRIVATE_SLOT.

In:
+        while (it.hasNext()){
+            d->socket.connectToHost(it.next(), port);
+            bool connectOk = d->socket.waitForConnected(d->timeout > -1 ? 
d->timeout * 1000 : -1);
+            if (connectOk) break;
+        }

You should be decrementing the timeout value there. Otherwise, if a target 
has 6 IP addresses, we could end up waiting up to 3 minutes before a 
timeout. I suggest:

	QTime timer;
	timer.start();
	while (it.hasNext()) {
		d->socket.connectToHost(it.next(), port);
		int timeout = -1;
		if (d->timeout > -1)
			timeout = d->timeout - timer.elapsed();
		if (!d->socket.waitForConnected(timeout))
			break;
	}

in HostInfoAgentPrivate::lookupHost
+    if(dnsCache.contains(hostName)) {
+        QPair<QHostInfo, QTime> info (*dnsCache.object(hostName));

Please void the double lookup of contains + object. Simply call object() 
and test for a 0 return.

The same in:
+    if (openQueries.contains(hostName)) {
+        Query* query = openQueries.value(hostName);

Please use an iterator and compare to openQueries.end() to see if the 
object is there.

You don't need Q_SIGNALS and Q_EMIT in a .cpp file. You can use signals: 
and emit. Not a big deal though.


In all, your patch is very good and very clever. The only drawback (= 
possible future improvement) is the fact that the IP addresses will be 
tried always in the same order. Right now, the DNS server will change the 
order in a round-robin fashion at every request.

-- 
  Thiago Macieira  -  thiago (AT) macieira.info - thiago (AT) kde.org
    PGP/GPG: 0x6EF45358; fingerprint:
    E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080701/c3040239/attachment.sig>