[Nepomuk] Review Request: Misc Filewatch fixes

[Sergey Borovkov]

> On Oct. 29, 2012, 11:18 a.m., David Faure wrote:
> > This assumes that QRegExp::exactMatch is re-entrant, i.e. that it doesn't update an internal cache on demand. I'm not so sure of that, the regexp parsing sounds like a very good candidate for on-demand parsing, unless you make extra sure that it's all done upfront.
> 
> Simeon Bird wrote:
>     The Qt documentation claims it is:
>     
>     http://qt-project.org/doc/qt-4.8/qregexp.html
>     "Note: All functions in this class are reentrant."
>     
>     (although it's true that I hadn't checked up to now, so that was still a good question)
> 
> David Faure wrote:
>     Never trust documentation :-)
>     
>     I had a look at the source code, and found indeed
>     1) on-demand initializations
>     2) no locking mechanism.
>     
>     So I wrote a test program, and guess what? It gives different output from one run to the next. It's memcheck-clean, but helgrind manages to spot the issue in some runs.
>     http://www.davidfaure.fr/2012/qregexp_race.cpp
>     http://www.davidfaure.fr/2012/qregexp-helgrind-log
>     The program is supposed to write out "true true true" but only does so 1 time out of 8 on average, in my tests. Nice, heh?
>     
>     => The documentation is wrong. I'll submit a docu fix to Qt...
> 
> David Faure wrote:
>     Wait, the documentation is right, the class is reentrant (i.e. different instances can be used in different threads).
>     
>     It's just not threadsafe (i.e. you can't use the same instance of QRegExp in different threads).
>     Therefore the documentation isn't wrong, but this patch is.
>     You must use one QRegExp per thread, or ensure exclusive use of the QRegExp instance.
>     
>     A QThreadStorage would make it possible to use one QRegExp per thread, but is rather difficult to handle if the regexp could change at runtime. So I think the only solution is "exclusive use of the QRegExp instance", i.e. QWriteLocker rather than QReadLocker (new conclusion: careful with what appears to be readonly).
> 
> Simeon Bird wrote:
>     Aha! Ok, you're quite right. I see also that I was confused about the difference between POSIX's definition of re-entrant and Qt's...
>     I'll make it a WriteLocker and add a comment in v2. Thanks for teaching me C++ once again :)

What do you mean difference in definition? If I am not mistaken re-entrant function can't use global data - and you are using just that. All class members have explicit 'this' parameter passed to to function.


- Sergey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107082/#review21082
-----------------------------------------------------------


On Oct. 27, 2012, 6:29 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107082/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2012, 6:29 p.m.)
> 
> 
> Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.
> 
> 
> Description
> -------
> 
> Some fairly trivial misc improvements to the filewatch service. Probably don't make a big difference, but probably a good idea. 
> 
> - Use QReadWriteLock instead of QMutex in FileIndexerConfig, thus allowing multiple threads to call shouldFolderBeIndexed at once (not that we really do that right now).
> 
> - Add the IN_EXCL_UNLINK inotify flag. On recent (2.6.36) kernels, this means we don't generate events for files once
> they have been unlinked from the directory we are watching. Prevents waking up for some already-deleted temporary files.
> 
> 
> Diffs
> -----
> 
>   services/fileindexer/fileindexerconfig.h 7debaf3 
>   services/fileindexer/fileindexerconfig.cpp 5226a79 
>   services/filewatch/kinotify.h 6e3f1c0 
>   services/filewatch/kinotify.cpp 9868b90 
> 
> Diff: http://git.reviewboard.kde.org/r/107082/diff/
> 
> 
> Testing
> -------
> 
> Compiled, ran for a bit, didn't seem to break anything. 
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20121029/7cfb3cdd/attachment.html>