Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1c446273 authored by Glenn Kasten's avatar Glenn Kasten
Browse files

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
parent eef598c5

media/libnbaio/NBLog.cpp

+15 −3
Original line number Diff line number Diff line
@@ -996,6 +996,8 @@ bool NBLog::Reader::isIMemory(const sp<IMemory>& iMemory) const 
    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 @@ NBLog::Merger::Merger(const void *shared, size_t size): 
      {}



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 @@ bool operator>(const struct MergeItem &i1, const struct MergeItem &i2) { 


// 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 @@ void NBLog::Merger::merge() { 
    }

}



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) {}

media/libnbaio/include/NBLog.h

+11 −2
Original line number Diff line number Diff line
@@ -37,6 +37,8 @@ public: 


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 @@ public: 


    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 @@ public: 
    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 @@ class MergeReader : public Reader { 
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);

services/medialog/MediaLogService.h

+2 −0
Original line number Diff line number Diff line
@@ -56,6 +56,8 @@ private: 
    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;