> On May 2, 2013, 9:19 a.m., Vishesh Handa wrote: > > Looks pretty amazing!
Cheers! > On May 2, 2013, 9:19 a.m., Vishesh Handa wrote: > > services/filewatch/nepomukfilewatch.cpp, line 172 > > <http://git.reviewboard.kde.org/r/106748/diff/3/?file=141496#file141496line172> > > > > 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. Ok - I'll remove it. I am kind of in favour of moving this logic to filewatch, because otherwise it nepomukctl restart filewatch only watches the homedir. But absolutely, that's another patch. > On May 2, 2013, 9:19 a.m., Vishesh Handa wrote: > > services/filewatch/kinotify.cpp, line 125 > > <http://git.reviewboard.kde.org/r/106748/diff/3/?file=141494#file141494line125> > > > > 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? Yes....you're right, it should be the non-encoded path. Actually, thinking about it, it should indeed have been path all along, and I'm kind of surprised it ever worked properly (or, uh, compiled). > On May 2, 2013, 9:19 a.m., Vishesh Handa wrote: > > services/filewatch/kinotify.cpp, line 301 > > <http://git.reviewboard.kde.org/r/106748/diff/3/?file=141494#file141494line301> > > > > Please add some spaces - > > > > kDebug() << "Removing:" << dirIter->path(); Oops...I didn't mean to post that for review anyway. > On May 2, 2013, 9:19 a.m., Vishesh Handa wrote: > > services/filewatch/kinotify.cpp, line 306 > > <http://git.reviewboard.kde.org/r/106748/diff/3/?file=141494#file141494line306> > > > > spaces > > Ditto - Simeon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106748/#review31873 ----------------------------------------------------------- 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 > >
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
