> On March 26, 2013, 8:51 p.m., David Faure wrote: > > libnepomukcore/resource/resourcedata.cpp, line 454 > > <http://git.reviewboard.kde.org/r/109747/diff/1/?file=122256#file122256line454> > > > > This uses m_cache.value() outside of the m_dataMutex lock. > > Plus, at that point, m_cache.value(uri) is always 'value', isn't it? > > Should this use an "oldValue" local variable instead, set before m_cache is > > updated, and from within the lock?
Indeed it should. Don't know how that slipped past me... thanks! > On March 26, 2013, 8:51 p.m., David Faure wrote: > > libnepomukcore/resource/resourcemanager.cpp, line 414 > > <http://git.reviewboard.kde.org/r/109747/diff/1/?file=122257#file122257line414> > > > > What do you mean by "take" here? Lock? Surely if the calling function > > was locking the rm mutex before calling addToWatcher, this line would > > deadlock... Not sure what this comment means. The rm mutex is recursive, no? So if it is already locked, it is ok to take it a second time? Or did I misunderstand how recursive mutexes work? Not that we are doing that. What I meant though was that in a number of cases, m_initializedData will be altered just after addToWatcher is called, so the threads will be mostly serialised anyway. I can amend the comment. > On March 26, 2013, 8:51 p.m., David Faure wrote: > > libnepomukcore/resource/resourcedata.cpp, line 331 > > <http://git.reviewboard.kde.org/r/109747/diff/1/?file=122256#file122256line331> > > > > Is there a risk another thread could modify m_uri at this point? Or are > > we sure it's never empty when this is called? I believe this is ok, because m_uri is only set once, when initialising, which happens immediately before calling addToWatcher. The other call to addToWatcher is preceded by a check that m_uri is non-null, with the dataMutex locked. I can add a comment to this effect. - Simeon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109747/#review29917 ----------------------------------------------------------- On March 26, 2013, 7:54 p.m., Simeon Bird wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109747/ > ----------------------------------------------------------- > > (Updated March 26, 2013, 7:54 p.m.) > > > Review request for Nepomuk, David Faure and Vishesh Handa. > > > Description > ------- > > Am I ok to merge this to KDE 4.10? I would like to merge the actual bugfixes, > but if you want I could omit the performance fixes/cleanups, which are > 1e76eedb82ee363c9caf333fe221962db81d67c6 > 75028c00ec71ccd67a046c0d71072623c2e5af74 > 464590095981e7ded537901d56f84ca49aa49d74 > > . > > Thanks > > commit dbbea65fb00c14852034be867396aa055041d848 > Author: Simeon Bird <[email protected]> > Date: Sat Mar 16 19:11:19 2013 -0400 > > ResourceWatcher: switch to using KDbusConnectionPool > > commit 1e76eedb82ee363c9caf333fe221962db81d67c6 > Author: Simeon Bird <[email protected]> > Date: Thu Mar 14 23:17:31 2013 -0400 > > Tidy up ResourceData::propertyAdded. > > If an entry is not found in a QHash, it will return an empty list, > so we only need to check for contains one way. > > commit 75028c00ec71ccd67a046c0d71072623c2e5af74 > Author: David Faure <[email protected]> > Date: Thu Mar 14 14:27:47 2013 +0100 > > Rework determineUri so that the RM lock isn't held during the executeQuery > > commit cc8a925989193b813dc50090c8b4a05e46fbfbdc > Author: David Faure <[email protected]> > Date: Thu Mar 14 14:26:45 2013 +0100 > > fix lock order in resetAll > > commit aec508539182f7bed2819cd23d82ba9baa120c8c > Author: David Faure <[email protected]> > Date: Thu Mar 14 13:23:22 2013 +0100 > > early return if nothing to do (noop, just makes the code clearer) > > commit 877b40f1916a64916d0869be5744dbc525931edd > Author: Simeon Bird <[email protected]> > Date: Sat Mar 16 16:27:44 2013 -0400 > > Fix bad threading in ResourceManagerPrivate::addToWatcher dbus usage. > > Instead of doing MoveToThread, take the resourceManager mutex before > talking to the ResourceWatcher. This is a lot clearer and less racy. > It should not be too contended now the rm mutex is not held over the > socket communication. > > commit 464590095981e7ded537901d56f84ca49aa49d74 > Author: Simeon Bird <[email protected]> > Date: Fri Mar 8 22:08:17 2013 -0500 > > ResourceData: Change updateKickOffLists to take three arguments, the > uri, old and new values. > > This means that it does not need to be under the dataMutex anymore, > which in turn means that we no longer need to remember to lock the RM > mutex before calling it, just to remember to unlock the dataMutex. > This in turn means that we can add a property that isn't NIE::url() or > NAO::identifier to the Resource without taking the mutex at all. > > commit 1b82506d609866e87b9d49b47de473766b01f3d6 > Author: Simeon Bird <[email protected]> > Date: Sat Mar 9 03:25:09 2013 -0500 > > Remove unneeded parameter from ResourceData::resetAll > > > This addresses bug 305024. > http://bugs.kde.org/show_bug.cgi?id=305024 > > > Diffs > ----- > > libnepomukcore/datamanagement/resourcewatcher.cpp > f394ae8705fa882b9f4329f93ea7d18c1cfabc73 > libnepomukcore/resource/resource.cpp > 7158802cf1e1ba48da86f103aa9311c7acbe24fe > libnepomukcore/resource/resourcedata.h > 39508764682b798290c1ae5b74a2384f9cbcbf58 > libnepomukcore/resource/resourcedata.cpp > 7a979745ca22c88a3a73921d9c0e64b51e064c38 > libnepomukcore/resource/resourcemanager.cpp > 2b32be01176059932cc1c2cf3b1f348ed91e982b > > Diff: http://git.reviewboard.kde.org/r/109747/diff/ > > > Testing > ------- > > Compiled, ran, used activities for a while. > > > Thanks, > > Simeon Bird > >
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
