[Nepomuk] Review Request 106748: Filewatch:

Thu May 2 09:19:30 UTC 2013

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

Looks pretty amazing!

services/filewatch/kinotify.cpp
<http://git.reviewboard.kde.org/r/106748/#comment23773>

    Are you sure we should be sending the encoded path to the filterWatch function? Cause the way I see it - the filterWatch function calls stuff like FileIndexerConfig::shouldFolderBeWatched( path ) and that needs the non-encoded file path.

    .
    .

    Just checked - and we always seem to have been passing the encoded path to addWatch( .. ). Is that correct?

services/filewatch/kinotify.cpp
<http://git.reviewboard.kde.org/r/106748/#comment23767>

    weird spaces

services/filewatch/kinotify.cpp
<http://git.reviewboard.kde.org/r/106748/#comment23768>

    Please add some spaces -

    kDebug() << "Removing:" << dirIter->path();

services/filewatch/kinotify.cpp
<http://git.reviewboard.kde.org/r/106748/#comment23769>

    spaces

services/filewatch/nepomukfilewatch.cpp
<http://git.reviewboard.kde.org/r/106748/#comment23771>

    Is this required?

    The FileIndexer does inform the filewatch to add these watches after it starts up. Then again, we can move that logic over here.

    The original logic was that we do not want to overburden the number of watches, but since we are already adding the home directory, it doesn't make much sense.

    Though, I am a little worried about the same watches being added twice since we aren't checking if that path has already been watched when adding watches - This is not a problem when adding them via the FileIndexer cause FileWatch::addWatch does have a check.

    Anyway, please remove this. If you want to fix this it should go in another patch.

services/filewatch/nepomukfilewatch.cpp
<http://git.reviewboard.kde.org/r/106748/#comment23772>

    Please add the word Nepomuk over here. I'm not sure people will understand what the "FileWatch service is"

    Actually, maybe you should add "KDE Nepomuk Filewatch service ...."

- Vishesh Handa

On April 30, 2013, 3:02 a.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106748/
> -----------------------------------------------------------
> 
> (Updated April 30, 2013, 3:02 a.m.)
> 
> 
> Review request for Nepomuk, Sebastian Trueg and Vishesh Handa.
> 
> 
> Description
> -------
> 
> Filewatch:
> 
> Watch all indexed folders on service startup.
> This is needed so that nepomukctl restart filewatch
> watches all needed folders.
> 
> FileWatch: Use KAuth to automatically raise the inotify watch limit if we run out of watches.
> 
> When we run out of watches, use a KAuth action to double the inotify
> watch limit (by writing to /proc/sys/fs/inotify/max_user_watches).
> At the same time, make the new setting persist across reboots by writing
> /etc/sysctl.d/97-kde-nepomuk-filewatch-inotify.conf.
> 
> If for some reason raising the limit does not work, print a message to
> syslog.
> 
> While the limit is being raised, no new watches will be added, only
> queued. Adding of watches is automatically resumed if the limit is raised.
> 
> ==Potential issues==
> 
> 2. At the moment there is no way to turn this off, except by not using nepomuk or denying the user the requisite kauth permissions. This is the sort of thing that people complain about, but I can't really see any reason to want to do that - you'd be running nepomuk in a "known broken" configuration, which makes no sense.
> 
> 3. the action description string is "To avoid missing file changes, raise the folder watch limit", which could probably be improved.
> 
> 4. The method of making the change persist across reboots is to write a file to /etc/sysctl.d, which is a bit anti-social. (note that if /etc is not writable, it simply does nothing). /etc/sysctl.d should work on all systemd distros, debian (including derivatives such as ubuntu) and gentoo.
> 
> Part of me wants to make this a separate action, but as I understand it this would require a second prompt and a second authorization, which would be a bit annoying. Also, the user's file system isn't going away - if they wanted a larger limit once, they almost certainly want it again, so there are limited reasons for not wanting it permanent. But finer grained permissions are a Good Thing, so I'm not so sure about this.
> 
> 5. If the user has manually set the watch limit to a too-low number in sysctl.conf, it could potentially over-ride the file in /etc/sysctl.d, leading to the prompt appearing on every boot.
> 
> Also, I'd just like to mention that I was quite impressed at how easy to KAuth was to work with.
> 
> 
> Diffs
> -----
> 
>   services/filewatch/CMakeLists.txt 338fe8c2b008b1c898d71934e4de3028c0078fca 
>   services/filewatch/kinotify.h e795371d922d483bce29e9eea03c1eeb97738355 
>   services/filewatch/kinotify.cpp 94babfe437ddfa8c9318b8b29dd8c8a03a4e71b1 
>   services/filewatch/nepomukfilewatch.h 66e0112d909a2abefed48d0959323e7f32a5ff9b 
>   services/filewatch/nepomukfilewatch.cpp fbbf3db619516e296bc1e4aa07f53808fbe4a4c0 
>   services/filewatch/org.kde.nepomuk.filewatch.actions PRE-CREATION 
>   services/filewatch/raiselimit.h PRE-CREATION 
>   services/filewatch/raiselimit.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106748/diff/
> 
> 
> Testing
> -------
> 
> Compiled, ran, raised and lowered the limit a few times.
> 
> It seems to work pretty well.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130502/8f84acd3/attachment.html>