Change raw pointer mNamedReaders to reference
Using a reference for mNamedReaders ensures it is never updated.
Also:
- add FIXME related to lack of lock protection for mNamedReaders.
- add a few other FIXME
- add section breaks
Bug: 37153050
Test: builds OK
Change-Id: I8b80acb5cc943795becdc2d24debc11b09620753
diff --git a/media/libnbaio/NBLog.cpp b/media/libnbaio/NBLog.cpp
index 1d1d61b..d0ee696 100644
--- a/media/libnbaio/NBLog.cpp
+++ b/media/libnbaio/NBLog.cpp
@@ -996,6 +996,8 @@
return iMemory != 0 && mIMemory != 0 && iMemory->pointer() == mIMemory->pointer();
}
+// ---------------------------------------------------------------------------
+
void NBLog::appendTimestamp(String8 *body, const void *data) {
int64_t ts;
memcpy(&ts, data, sizeof(ts));
@@ -1209,6 +1211,8 @@
{}
void NBLog::Merger::addReader(const NBLog::NamedReader &reader) {
+ // FIXME This is called by binder thread in MediaLogService::registerWriter
+ // but the access to shared variable mNamedReaders is not yet protected by a lock.
mNamedReaders.push_back(reader);
}
@@ -1232,6 +1236,8 @@
// Merge registered readers, sorted by timestamp
void NBLog::Merger::merge() {
+ // FIXME This is called by merge thread
+ // but the access to shared variable mNamedReaders is not yet protected by a lock.
int nLogs = mNamedReaders.size();
std::vector<std::unique_ptr<NBLog::Reader::Snapshot>> snapshots(nLogs);
std::vector<NBLog::EntryIterator> offsets(nLogs);
@@ -1266,19 +1272,25 @@
}
}
-const std::vector<NBLog::NamedReader> *NBLog::Merger::getNamedReaders() const {
- return &mNamedReaders;
+const std::vector<NBLog::NamedReader>& NBLog::Merger::getNamedReaders() const {
+ // FIXME This is returning a reference to a shared variable that needs a lock
+ return mNamedReaders;
}
+// ---------------------------------------------------------------------------
+
NBLog::MergeReader::MergeReader(const void *shared, size_t size, Merger &merger)
: Reader(shared, size), mNamedReaders(merger.getNamedReaders()) {}
void NBLog::MergeReader::handleAuthor(const NBLog::AbstractEntry &entry, String8 *body) {
int author = entry.author();
- const char* name = (*mNamedReaders)[author].name();
+ // FIXME Needs a lock
+ const char* name = mNamedReaders[author].name();
body->appendFormat("%s: ", name);
}
+// ---------------------------------------------------------------------------
+
NBLog::MergeThread::MergeThread(NBLog::Merger &merger)
: mMerger(merger),
mTimeoutUs(0) {}
diff --git a/media/libnbaio/include/NBLog.h b/media/libnbaio/include/NBLog.h
index 403f692..79e1679 100644
--- a/media/libnbaio/include/NBLog.h
+++ b/media/libnbaio/include/NBLog.h
@@ -37,6 +37,8 @@
typedef uint64_t log_hash_t;
+// FIXME Everything needed for client (writer API and registration) should be isolated
+// from the rest of the implementation.
class Writer;
class Reader;
@@ -319,6 +321,7 @@
virtual ~Writer();
+ // FIXME needs comments, and some should be private
virtual void log(const char *string);
virtual void logf(const char *fmt, ...) __attribute__ ((format (printf, 2, 3)));
virtual void logvf(const char *fmt, va_list ap);
@@ -500,11 +503,15 @@
void addReader(const NamedReader &reader);
// TODO add removeReader
void merge();
- const std::vector<NamedReader> *getNamedReaders() const;
+ // FIXME This is returning a reference to a shared variable that needs a lock
+ const std::vector<NamedReader>& getNamedReaders() const;
private:
// vector of the readers the merger is supposed to merge from.
// every reader reads from a writer's buffer
+ // FIXME Needs to be protected by a lock
std::vector<NamedReader> mNamedReaders;
+
+ // TODO Need comments on all of these
uint8_t *mBuffer;
Shared * const mShared;
std::unique_ptr<audio_utils_fifo> mFifo;
@@ -515,7 +522,9 @@
public:
MergeReader(const void *shared, size_t size, Merger &merger);
private:
- const std::vector<NamedReader> *mNamedReaders;
+ // FIXME Needs to be protected by a lock,
+ // because even though our use of it is read-only there may be asynchronous updates
+ const std::vector<NamedReader>& mNamedReaders;
// handle author entry by looking up the author's name and appending it to the body
// returns number of bytes read from fmtEntry
void handleAuthor(const AbstractEntry &fmtEntry, String8 *body);
diff --git a/services/medialog/MediaLogService.h b/services/medialog/MediaLogService.h
index e9211b4..39d9cc0 100644
--- a/services/medialog/MediaLogService.h
+++ b/services/medialog/MediaLogService.h
@@ -56,6 +56,8 @@
Mutex mLock;
Vector<NBLog::NamedReader> mNamedReaders; // protected by mLock
+
+ // FIXME Need comments on all of these, especially about locking
NBLog::Shared *mMergerShared;
NBLog::Merger mMerger;
NBLog::MergeReader mMergeReader;