kdecore: rewrite KLockFile

the PID check was racy, bonus points for not writing data at all to the
lock and not reading it meaning less disk I/O

oh, yes - by using O_CLOEXEC the lock is stale-safe

Signed-off-by: Ivailo Monev <xakepa10@gmail.com>

kdecore/config/kconfigini.cpp

@@ -594,10 +594,7 @@ bool KConfigIniBackend::lock(const KComponentData& componentData)
         m_lockfile = new KLockFile(filePath());
     }
 
-    if (m_lockfile->lock() == KLockFile::LockStale) {
-        // attempt to break the lock
-        m_lockfile->lock(KLockFile::ForceFlag);
-    }
+    m_lockfile->lock();
     return m_lockfile->isLocked();
 }
 

kdecore/io/klockfile.h

@@ -46,36 +46,14 @@ public:
     ~KLockFile();
 
     /*!
-        @brief Possible return values of the lock function
+        @brief Attempts to acquire the lock
     */
-    enum LockResult {
-        //! @brief Lock was acquired successfully
-        LockOK = 0,
-
-        //! @brief The lock could not be acquired because it is held by another process
-        LockFail,
-
-        //! @brief The lock could not be acquired due to an error
-        LockError,
-
-        //! @brief A stale lock has been detected, i.e. the process that owned the lock has crashed
-        LockStale
-    };
-
-    enum LockFlag {
-        //! @brief Return immediately, do not wait for the lock to become available
-        NoBlockFlag = 1,
-
-        //! @brief Automatically remove a lock when it is detected to be stale
-        ForceFlag = 2
-    };
-    Q_DECLARE_FLAGS(LockFlags, LockFlag)
+    bool tryLock();
 
     /*!
-        @brief Attempt to acquire the lock
-        @param flags A set of @ref LockFlag values OR'ed together
+        @brief Acquires the lock
     */
-    LockResult lock(LockFlags flags = LockFlags());
+    void lock();
 
     /*!
         @brief Returns whether the lock is held or not
@@ -87,17 +65,9 @@ public:
     */
     void unlock();
 
-    /*!
-        @brief Returns the pid of the process holding the lock.
-        @returns false if the pid could not be determined
-    */
-    bool getLockInfo(qint64 &pid);
-
 private:
     Q_DISABLE_COPY(KLockFile);
     KLockFilePrivate *const d;
 };
 
-Q_DECLARE_OPERATORS_FOR_FLAGS(KLockFile::LockFlags)
-
-#endif
+#endif // KLOCKFILE_H

kdecore/io/klockfile_unix.cpp

@@ -37,76 +37,15 @@ class KLockFilePrivate
 public:
     KLockFilePrivate();
 
-    KLockFile::LockResult tryLock();
-
     QByteArray m_lockfile;
     int m_lockfd;
-    qint64 m_pid;
 };
 
 KLockFilePrivate::KLockFilePrivate()
-    : m_lockfd(-1),
-    m_pid(-1)
+    : m_lockfd(-1)
 {
-    m_pid = static_cast<qint64>(::getpid());
 }
 
-KLockFile::LockResult KLockFilePrivate::tryLock()
-{
-    if (m_lockfd != -1) {
-        return KLockFile::LockOK;
-    }
-
-    char lockbuffer[256];
-    ::memset(lockbuffer, 0, sizeof(lockbuffer) * sizeof(char));
-    m_lockfd = KDE_open(m_lockfile.constData(), O_WRONLY | O_CREAT | O_EXCL, 0644);
-    if (m_lockfd == -1) {
-        const int savederrno = errno;
-        if (savederrno == EEXIST) {
-            const int infofile = KDE_open(m_lockfile.constData(), O_RDONLY);
-            if (Q_UNLIKELY(infofile == -1)) {
-                kWarning() << "Could not open lock file";
-                return KLockFile::LockFail;
-            }
-            const int inforesult = QT_READ(infofile, lockbuffer, sizeof(lockbuffer));
-            QT_CLOSE(infofile);
-            if (Q_UNLIKELY(inforesult <= 0)) {
-                kWarning() << "Could not read lock file";
-                return KLockFile::LockFail;
-            }
-            qint64 lockpid = 0;
-            ::sscanf(lockbuffer, "%lld", &lockpid);
-            if (Q_UNLIKELY(lockpid <= 0)) {
-                kWarning() << "Invalid lock information";
-                return KLockFile::LockFail;
-            }
-            kDebug() << "Lock" << m_lockfile << "held by" << lockpid;
-
-            if (::kill(lockpid, 0) == -1 && errno == ESRCH) {
-                kWarning() << "Stale lock" << m_lockfile << "held by" << lockpid;
-                return KLockFile::LockStale;
-            }
-
-            return KLockFile::LockFail;
-        }
-
-        kWarning() << "Could not create lock file" << qt_error_string(savederrno);
-        return KLockFile::LockError;
-    }
-
-    ::memset(lockbuffer, 0, sizeof(lockbuffer) * sizeof(char));
-    ::snprintf(lockbuffer, sizeof(lockbuffer), "%lld", m_pid);
-    const int lockbufferlen = qstrlen(lockbuffer);
-    if (Q_UNLIKELY(QT_WRITE(m_lockfd, lockbuffer, lockbufferlen) != lockbufferlen)) {
-        const int savederrno = errno;
-        kWarning() << "Could not write lock information" << qt_error_string(savederrno);
-        return KLockFile::LockError;
-    }
-
-    return KLockFile::LockOK;
-}
-
-
 KLockFile::KLockFile(const QString &file)
     : d(new KLockFilePrivate())
 {
@@ -120,25 +59,26 @@ KLockFile::~KLockFile()
     delete d;
 }
 
-KLockFile::LockResult KLockFile::lock(LockFlags options)
+bool KLockFile::tryLock()
 {
-tryagain:
-    KLockFile::LockResult result = d->tryLock();
-    if (result == KLockFile::LockStale && options & KLockFile::ForceFlag) {
-        kWarning() << "Deleting stale lock file" << d->m_lockfile;
-        if (Q_UNLIKELY(::unlink(d->m_lockfile.constData()) == -1)) {
-            const int savederrno = errno;
-            kWarning() << "Could not remove lock file" << qt_error_string(savederrno);
-        }
-        result = d->tryLock();
+    if (d->m_lockfd != -1) {
+        return true;
     }
-    if (!(options & KLockFile::NoBlockFlag) && result == KLockFile::LockFail) {
-        kDebug() << "Retrying to lock after" << (KLOCKFILE_TIMEOUT + KLOCKFILE_SLEEPTIME);
+
+#ifdef O_CLOEXEC
+    d->m_lockfd = KDE_open(d->m_lockfile.constData(), O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0644);
+#else
+    d->m_lockfd = KDE_open(d->m_lockfile.constData(), O_WRONLY | O_CREAT | O_EXCL, 0644);
+#endif
+    return (d->m_lockfd != -1);
+}
+
+void KLockFile::lock()
+{
+    while (!tryLock()) {
         QCoreApplication::processEvents(QEventLoop::AllEvents, KLOCKFILE_TIMEOUT);
         QThread::msleep(KLOCKFILE_SLEEPTIME);
-        goto tryagain;
     }
-    return result;
 }
 
 bool KLockFile::isLocked() const
@@ -157,12 +97,3 @@ void KLockFile::unlock()
         d->m_lockfd = -1;
     }
 }
-
-bool KLockFile::getLockInfo(qint64 &pid)
-{
-    if (d->m_lockfd == -1) {
-        return false;
-    }
-    pid = d->m_pid;
-    return true;
-}

kdecore/tests/klockfile_testlock.cpp

@@ -29,14 +29,14 @@ int main(int argc, char *argv[])
    KComponentData cData(&about);
 
    if (argc <= 1) {
-       return KLockFile::LockError;
+       return false;
    }
 
    const QString lockName = QString::fromLocal8Bit(argv[1]);
 
    KLockFile lockFile(lockName);
    if (lockFile.isLocked()) {
-       return KLockFile::LockError;
+       return false;
    }
-   return lockFile.lock(KLockFile::NoBlockFlag);
+   return lockFile.tryLock();
 }

kdecore/tests/klockfiletest.cpp

@@ -28,19 +28,6 @@
 
 // TODO test locking from two different threads
 
-QT_BEGIN_NAMESPACE
-namespace QTest {
-template<>
-char* toString(const KLockFile::LockResult &result)
-    {
-        static const char *const strings[] = {
-                "LockOK", "LockFail", "LockError", "LockStale"
-        };
-        return qstrdup(strings[result]);
-    }
-}
-QT_END_NAMESPACE
-
 static const QString lockName = "klockfiletest";
 static const char *const lockNameFull = "klockfiletest.klockfile";
 
@@ -56,44 +43,27 @@ void Test_KLockFile::cleanupTestCase()
     lockFile = nullptr;
 }
 
-static KLockFile::LockResult testLockFromProcess(const QString& lockName)
+static bool testLockFromProcess(const QString& lockName)
 {
     const int ret = QProcess::execute(KDEBINDIR "/kdecore-klockfile_testlock", QStringList() << lockName);
-    return KLockFile::LockResult(ret);
+    return bool(ret);
 }
 
 void Test_KLockFile::testLock()
 {
     QVERIFY(!lockFile->isLocked());
-    QCOMPARE(lockFile->lock(), KLockFile::LockOK);
+    QCOMPARE(lockFile->tryLock(), true);
     QVERIFY(lockFile->isLocked());
 
     // Try to lock it again, should fail
     KLockFile *lockFile2 = new KLockFile(lockName);
     QVERIFY(!lockFile2->isLocked());
-    QCOMPARE(lockFile2->lock(KLockFile::NoBlockFlag), KLockFile::LockFail);
+    QCOMPARE(lockFile2->tryLock(), false);
     QVERIFY(!lockFile2->isLocked());
     delete lockFile2;
 
     // Also try from a different process.
-    QCOMPARE(testLockFromProcess(lockName), KLockFile::LockFail);
-}
-
-void Test_KLockFile::testLockInfo()
-{
-    QVERIFY(lockFile->isLocked());
-
-    KLockFile lf(lockName);
-    QCOMPARE(lf.lock(KLockFile::NoBlockFlag), KLockFile::LockFail);
-    QVERIFY(!lf.isLocked());
-
-    qint64 pid = -1;
-    QVERIFY(!lf.getLockInfo(pid));
-    QCOMPARE(pid, qint64(-1));
-
-    pid = -1;
-    QVERIFY(lockFile->getLockInfo(pid));
-    QCOMPARE(pid, static_cast<qint64>(::getpid()));
+    QCOMPARE(testLockFromProcess(lockName), false);
 }
 
 void Test_KLockFile::testUnlock()
@@ -103,23 +73,4 @@ void Test_KLockFile::testUnlock()
     QVERIFY(!lockFile->isLocked());
 }
 
-
-void Test_KLockFile::testStaleNoBlockFlag()
-{
-    QFile f(QString::fromLatin1(lockNameFull));
-    f.open(QIODevice::WriteOnly);
-    QTextStream stream(&f);
-    stream << QString::number(111222);
-    stream.flush();
-    f.close();
-
-    KLockFile* lockFile2 = new KLockFile(lockName);
-    QVERIFY(!lockFile2->isLocked());
-    QCOMPARE(lockFile2->lock(KLockFile::NoBlockFlag), KLockFile::LockStale);
-    QCOMPARE(lockFile2->lock(KLockFile::NoBlockFlag|KLockFile::ForceFlag), KLockFile::LockOK);
-
-    QVERIFY(lockFile2->isLocked());
-    delete lockFile2;
-}
-
 QTEST_KDEMAIN_CORE(Test_KLockFile)

kdecore/tests/klockfiletest.h

@@ -31,9 +31,7 @@ private Q_SLOTS:
     void initTestCase();
     void cleanupTestCase();
     void testLock();
-    void testLockInfo();
     void testUnlock();
-    void testStaleNoBlockFlag();
 };
 
 #endif // KLOCKFILETEST_H