Review Fixes

Review Fixes

Author: Renu-Priya411

qdlt/dltmessagematcher.cpp

@@ -16,51 +16,30 @@ bool DltMessageMatcher::match(const QDltMsg &msg, const QString &searchText) con
 
 bool DltMessageMatcher::match(const QDltMsg &msg, const Pattern& pattern) const
 {
-    if (!matchAppId(msg.getApid()) || !matchCtxId(msg.getCtid()))
+    if (!passesPreFilters(msg))
         return false;
 
-    if (!matchTimestampRange(msg.getTimestamp())) {
-        return false;
-    }
-
-    if (m_timeRangeMs)
-    {
-        const qint64 timestampMSecsSinceEpoch = msg.getTime() * 1000 + msg.getMicroseconds() / 1000;
-        if (!matchTimeRangeMs(timestampMSecsSinceEpoch))
-            return false;
-    }
+    if (std::holds_alternative<QRegularExpression>(pattern))
+        return matchHeaderAndPayload(msg, std::get<QRegularExpression>(pattern));
 
-    if (std::holds_alternative<QRegularExpression>(pattern)) {
-        bool matchFound = false;
-        if (m_headerSearchEnabled) {
-            auto header = msg.toStringHeader();
-            if (m_messageIdFormat)
-                header += ' ' + QString::asprintf(m_messageIdFormat->toUtf8(), msg.getMessageId());
-            matchFound = header.contains(std::get<QRegularExpression>(pattern));
-        }
-
-        if (matchFound)
-            return true;
-
-        if (m_payloadSearchEnabled) {
-            const auto payload = msg.toStringPayload();
-            matchFound = payload.contains(std::get<QRegularExpression>(pattern));
-        }
+    return matchHeaderAndPayload(msg, std::get<QString>(pattern));
+}
 
-        return matchFound;
-    }
+bool DltMessageMatcher::matchText(const QDltMsg &msg, const QString &searchText) const
+{
+    if (!passesPreFilters(msg))
+        return false;
 
-    return matchText(msg, std::get<QString>(pattern));
+    return matchHeaderAndPayload(msg, searchText);
 }
 
-bool DltMessageMatcher::matchText(const QDltMsg &msg, const QString &searchText) const
+bool DltMessageMatcher::passesPreFilters(const QDltMsg &msg) const
 {
     if (!matchAppId(msg.getApid()) || !matchCtxId(msg.getCtid()))
         return false;
 
-    if (!matchTimestampRange(msg.getTimestamp())) {
+    if (!matchTimestampRange(msg.getTimestamp()))
         return false;
-    }
 
     if (m_timeRangeMs)
     {
@@ -69,6 +48,11 @@ bool DltMessageMatcher::matchText(const QDltMsg &msg, const QString &searchText)
             return false;
     }
 
+    return true;
+}
+
+bool DltMessageMatcher::matchHeaderAndPayload(const QDltMsg &msg, const QString &searchText) const
+{
     if (m_headerSearchEnabled) {
         auto header = msg.toStringHeader();
         if (m_messageIdFormat)
@@ -85,6 +69,24 @@ bool DltMessageMatcher::matchText(const QDltMsg &msg, const QString &searchText)
     return false;
 }
 
+bool DltMessageMatcher::matchHeaderAndPayload(const QDltMsg &msg, const QRegularExpression &pattern) const
+{
+    if (m_headerSearchEnabled) {
+        auto header = msg.toStringHeader();
+        if (m_messageIdFormat)
+            header += ' ' + QString::asprintf(m_messageIdFormat->toUtf8(), msg.getMessageId());
+        if (header.contains(pattern))
+            return true;
+    }
+
+    if (m_payloadSearchEnabled) {
+        const auto payload = msg.toStringPayload();
+        return payload.contains(pattern);
+    }
+
+    return false;
+}
+
 bool DltMessageMatcher::matchAppId(const QString& appId) const
 {
     return m_appId.isEmpty() || appId.compare(m_appId, m_caseSensitivity) == 0;

qdlt/dltmessagematcher.h

@@ -56,6 +56,10 @@ class QDLT_EXPORT DltMessageMatcher
     bool match(const QDltMsg& message, const Pattern& pattern) const;
 private:
     bool matchText(const QDltMsg& message, const QString& searchText) const;
+    // Common appId/ctxId/timestamp/time-range checks shared by the regex and text match paths.
+    bool passesPreFilters(const QDltMsg& message) const;
+    bool matchHeaderAndPayload(const QDltMsg& message, const QString& searchText) const;
+    bool matchHeaderAndPayload(const QDltMsg& message, const QRegularExpression& pattern) const;
     bool matchAppId(const QString& appId) const;
     bool matchCtxId(const QString& ctxId) const;
     bool matchTimestampRange(unsigned int ts) const;

qdlt/qdltfile.cpp

@@ -110,7 +110,7 @@ void QDltFile::setDltIndex(QVector<qint64> &_indexAll, int num)
     bumpSearchSnapshotGenerationLocked();
 }
 
-int QDltFile::size() const
+int QDltFile::sizeLocked() const
 {
     int size=0;
     for(int num=0;num<files.size();num++)
@@ -122,8 +122,15 @@ int QDltFile::size() const
     return size;
 }
 
+int QDltFile::size() const
+{
+    QMutexLocker locker(&mutexQDlt);
+    return sizeLocked();
+}
+
 qint64 QDltFile::fileSize() const
 {
+    QMutexLocker locker(&mutexQDlt);
     qint64 size=0;
 
     for(int num=0;num<files.size();num++)
@@ -141,11 +148,7 @@ int QDltFile::sizeFilter() const
     if(filterFlag)
         return indexFilter.size();
 
-    int total = 0;
-    for(int num = 0; num < files.size(); ++num)
-        total += files[num]->indexAll.size();
-
-    return total;
+    return sizeLocked();
 }
 
 int QDltFile::calculateHeaderSize(quint8 htyp)
@@ -203,13 +206,20 @@ bool QDltFile::open(QString _filename, bool append)
 
     /* create new file item */
     QDltFileItem *item = new QDltFileItem();
-    files.append(item);
 
     /* set new filename */
     item->infile.setFileName(_filename);
 
     /* open the log file read only */
-    if(item->infile.open(QIODevice::ReadOnly)==false)
+    const bool opened = item->infile.open(QIODevice::ReadOnly);
+
+    {
+        QMutexLocker locker(&mutexQDlt);
+        files.append(item);
+        bumpSearchSnapshotGenerationLocked();
+    }
+
+    if(!opened)
     {
         /* open file failed */
         qWarning() << "open of file" << _filename << "failed";
@@ -638,6 +648,7 @@ void QDltFile::addFilterIndex (int index)
 
 QString QDltFile::getFileName(int num)
 {
+    QMutexLocker locker(&mutexQDlt);
     if(num<0 || num>=files.size())
         return QString();
 
@@ -646,6 +657,7 @@ QString QDltFile::getFileName(int num)
 
 int QDltFile::getFileMsgNumber(int num) const
 {
+    QMutexLocker locker(&mutexQDlt);
     if(num<0 || num>=files.size())
         return -1;
 
@@ -663,6 +675,12 @@ void QDltFile::close()
 }
 
 QByteArray QDltFile::getMsg(int index) const
+{
+    QMutexLocker locker(&mutexQDlt);
+    return getMsgLocked(index);
+}
+
+QByteArray QDltFile::getMsgLocked(int index) const
 {
     QByteArray buf;
     int num = 0;
@@ -702,8 +720,6 @@ QByteArray QDltFile::getMsg(int index) const
         return QByteArray();
     }
 
-    mutexQDlt.lock();
-
     QDltFileItem* file = files[num];
     const QDltFileItem* const_file = file;
     qint64 positionForIndex = const_file->indexAll[index];
@@ -712,7 +728,6 @@ QByteArray QDltFile::getMsg(int index) const
     if ( false == file->infile.seek(positionForIndex) )
     {
         qDebug() << "Seek error on " << positionForIndex << file->infile.fileName() << __FILE__ << __LINE__;
-        mutexQDlt.unlock();
         buf.clear();
         return buf;
     }
@@ -738,8 +753,6 @@ QByteArray QDltFile::getMsg(int index) const
          buf = file->infile.read(cal_index);
     }
 
-    mutexQDlt.unlock();
-
     /* return DLT message buffer */
     return buf;
 }
@@ -1003,7 +1016,8 @@ QVector<qint64> QDltFile::mergeIndexFilterBaseWithMarkers(const QSet<qint64> &ma
     if(markerSet.isEmpty())
         return indexFilterBase;
 
-    const qint64 maxIdx = static_cast<qint64>(size());
+    // Called under mutexQDlt via recomputeEffectiveIndexFilterLocked(); use lock-free helpers.
+    const qint64 maxIdx = static_cast<qint64>(sizeLocked());
 
     // Fast membership check: which indices are already present in the base filter output.
     QSet<qint64> present;
@@ -1047,7 +1061,7 @@ QVector<qint64> QDltFile::mergeIndexFilterBaseWithMarkers(const QSet<qint64> &ma
 
         if(idx >= 0 && idx < maxIdx && (sortByTime || sortByTimestamp))
         {
-            const QByteArray data = getMsg(static_cast<int>(idx));
+            const QByteArray data = getMsgLocked(static_cast<int>(idx));
             if(!data.isEmpty())
             {
                 QDltMsg msg;

qdlt/qdltfile.h

@@ -363,8 +363,12 @@ class QDLT_EXPORT QDltFile : public QDlt
 private:
     // Calculates total storage, message, and payload sizes for all indexed DLT messages.
     void calculateTotalSizes();
-  void recomputeEffectiveIndexFilterLocked();
-  void bumpSearchSnapshotGenerationLocked();
+    void recomputeEffectiveIndexFilterLocked();
+    void bumpSearchSnapshotGenerationLocked();
+    // Lock-free counterparts of size()/getMsg(int) for callers that already hold mutexQDlt
+    // (e.g. mergeIndexFilterBaseWithMarkers, invoked from the *Locked recompute path).
+    int sizeLocked() const;
+    QByteArray getMsgLocked(int index) const;
 
     //! Mutex to lock critical path for infile
     mutable QMutex mutexQDlt;

qdlt/searchsnapshot.cpp

@@ -62,13 +62,3 @@ std::shared_ptr<const SearchSnapshot> SearchSnapshotManager::capture(const QDltF
     m_cachedSnapshot = snapshot;
     return snapshot;
 }
-
-void SearchSnapshotManager::invalidate(const QDltFile *file)
-{
-    QMutexLocker locker(&m_mutex);
-    if(file != nullptr && m_cachedFile != file)
-        return;
-
-    m_cachedFile = nullptr;
-    m_cachedSnapshot.reset();
-}

qdlt/searchsnapshot.h

@@ -61,12 +61,11 @@ class QDLT_EXPORT SearchSnapshotManager
 {
 public:
     std::shared_ptr<const SearchSnapshot> capture(const QDltFile *file);
-    void invalidate(const QDltFile *file = nullptr);
 
 private:
     mutable QMutex m_mutex;
     const QDltFile *m_cachedFile{nullptr};
     std::shared_ptr<const SearchSnapshot> m_cachedSnapshot;
 };
 
-#endif // SEARCHSNAPSHOT_H
+#endif // SEARCHSNAPSHOT_H