[kde-freebsd] QFilesSystemWatcher instead of polling/FAM

[Raphael Kubo da Costa]

On Monday 09 August 2010 06:01:56 Tijl Coosemans wrote:
> On Saturday 07 August 2010 04:34:55 Raphael Kubo da Costa wrote:
> > Are you planning on sending these patches upstream?
> 
> For the Qt patch there's a merge request here:
> http://qt.gitorious.org/qt/qt/merge_requests/2425

Excellent!

> I've attached what I believe to be the final version of the kdelibs
> patch. It uses polling by default for NFS mounts now and fixes a few
> other things. Unless someone finds a problem with it, I'll submit that
> as well.

Here's an informal review of the patch before you submit it to KDE's 
ReviewBoard. First of all, please try to compile the patch with trunk as well: 
the patch as is won't apply cleanly because kdirwatch has been moved to 
kdecore/io.

--- kio/CMakeLists.txt.orig	2010-06-24 14:08:17.000000000 +0200
+++ kio/CMakeLists.txt	2010-06-24 14:08:42.000000000 +0200
@@ -28,11 +28,9 @@
 
 check_include_files(sys/inotify.h SYS_INOTIFY_H_FOUND)
 macro_bool_to_01(SYS_INOTIFY_H_FOUND HAVE_SYS_INOTIFY_H)
-if(WIN32)
- # currently for win32 only --> enable it for all in 4.1?
- option(USE_QFILESYSTEMWATCHER "Use QFileSystemWatcher instead polling for 
KDirWatch" ON)
- macro_bool_to_01(USE_QFILESYSTEMWATCHER HAVE_QFILESYSTEMWATCHER)
-endif(WIN32)
+
+option(USE_QFILESYSTEMWATCHER "Use QFileSystemWatcher instead polling for 
KDirWatch" ON)
+macro_bool_to_01(USE_QFILESYSTEMWATCHER HAVE_QFILESYSTEMWATCHER)

Unless you're sure it's better to always enable QFileSystemWatcher for all 
platforms (including Linux, OS X, OpenSolaris etc), that if() should be 
changed instead of removed -- for example, use if (WIN32 OR CMAKE_SYSTEM_NAME 
MATCHES "FreeBSD").

--- kio/kio/kdirwatch_p.h.orig  2010-06-24 15:21:37.000000000 +0200
+++ kio/kio/kdirwatch_p.h   2010-06-24 15:21:58.000000000 +0200
@@ -102,7 +102,7 @@
   QHash<QString,QFileSystemWatcher*> m_paths;
 };
 #else
-typedef KFileSystemWatcher QFileSystemWatcher;
+typedef QFileSystemWatcher KFileSystemWatcher;
 #endif
 #endif

Is this really wanted for all platforms?

-    fsWatcher->removePath(e->path);
+    if ( e->m_status == Normal ) {
+      fsWatcher->removePath(e->path);
+      kDebug(7001).nospace() << "Cancelled QFSWatch for " << e->path;
+    }
+    else {
+      if (e->isDir)
+        removeEntry(0, QDir::cleanPath(e->path+"/.."), e);
+      else
+        removeEntry(0, QFileInfo(e->path).absolutePath(), e);
+    }

kdelibs coding style:

  if (foo) {
      bar(1 + 1);
  }

is preferred over
  if ( foo )
     bar(1+1);

-    Entry entry = *it;  // deep copy to not point to uninialized data (can 
happen inside emitEvent() )
-    Entry *e = &entry;
+    Entry *e = &(*it);

It may be nice to keep the comment.

+    if(ev == Created) {
+      if(!useQFSWatch(e))
+#ifdef HAVE_SYS_INOTIFY_H
+        if(!useINotify(e))
+#endif
+          useStat(e);
+    } else

Same comment above about coding style applies.

If possible, it'd be good to add some unit tests as well.

Cheers,
Raphael