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

Commit a863b1a0 authored by Tom Cherry's avatar Tom Cherry
Browse files

logd: simplify Clear() + Prune() logic 

Clear() and Prune() return a boolean indicating whether or not their
operations failed because the log buffer was 'busy'.  This means that
they return false upon success and true upon failure, which is not
intuitive.

This change inverts their return value to simply be true if they were
successful or false otherwise.  It also simplifies the return value of
ChattyLogBuffer::Prune() to true if the requested number of rows have
been pruned or if all rows in the log buffer have been pruned, and
otherwise return false.

Test: logging unit tests
Test: clearing works even under logging pressure
Change-Id: I346bb945496ef62bf8e973298f81c5163f49bc57
parent 71de690f

logd/ChattyLogBuffer.cpp

+4 −9
Original line number Diff line number Diff line
@@ -327,7 +327,6 @@ class LogBufferElementLast { 
//

bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_uid) {

    LogReaderThread* oldest = nullptr;

    bool busy = false;

    bool clearAll = pruneRows == ULONG_MAX;



    auto reader_threads_lock = std::lock_guard{reader_list()->reader_threads_lock()};
@@ -359,17 +358,16 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u 
            }



            if (oldest && oldest->start() <= element.sequence()) {

                busy = true;

                KickReader(oldest, id, pruneRows);

                break;

                return false;

            }



            it = Erase(it);

            if (--pruneRows == 0) {

                break;

                return true;

            }

        }

        return busy;

        return true;

    }



    // prune by worst offenders; by blacklist, UID, and by PID of system UID
@@ -440,7 +438,6 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u 
            LogBufferElement& element = *it;



            if (oldest && oldest->start() <= element.sequence()) {

                busy = true;

                // Do not let chatty eliding trigger any reader mitigation

                break;

            }
@@ -577,7 +574,6 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u 
        }



        if (oldest && oldest->start() <= element.sequence()) {

            busy = true;

            if (!whitelist) KickReader(oldest, id, pruneRows);

            break;

        }
@@ -605,7 +601,6 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u 
            }



            if (oldest && oldest->start() <= element.sequence()) {

                busy = true;

                KickReader(oldest, id, pruneRows);

                break;

            }
@@ -615,5 +610,5 @@ bool ChattyLogBuffer::Prune(log_id_t id, unsigned long pruneRows, uid_t caller_u 
        }

    }



    return (pruneRows > 0) && busy;

    return pruneRows == 0 || it == logs().end();

}

logd/CommandListener.cpp

+1 −1
Original line number Diff line number Diff line
@@ -82,7 +82,7 @@ int CommandListener::ClearCmd::runCommand(SocketClient* cli, int argc, 
        return 0;

    }



    cli->sendMsg(buf()->Clear((log_id_t)id, uid) ? "busy" : "success");

    cli->sendMsg(buf()->Clear((log_id_t)id, uid) ? "success" : "busy");

    return 0;

}

logd/SimpleLogBuffer.cpp

+29 −37
Original line number Diff line number Diff line
@@ -212,24 +212,26 @@ bool SimpleLogBuffer::FlushTo( 
    return true;

}



// clear all rows of type "id" from the buffer.

bool SimpleLogBuffer::Clear(log_id_t id, uid_t uid) {

    bool busy = true;

    // If it takes more than 4 tries (seconds) to clear, then kill reader(s)

    for (int retry = 4;;) {

        if (retry == 1) {  // last pass

            // Check if it is still busy after the sleep, we say prune

            // one entry, not another clear run, so we are looking for

            // the quick side effect of the return value to tell us if

            // we have a _blocked_ reader.

    // Try three times to clear, then disconnect the readers and try one final time.

    for (int retry = 0; retry < 3; ++retry) {

        {

            auto lock = std::lock_guard{lock_};

                busy = Prune(id, 1, uid);

            if (Prune(id, ULONG_MAX, uid)) {

                return true;

            }

        }

        sleep(1);

    }

    // Check if it is still busy after the sleep, we try to prune one entry, not another clear run,

    // so we are looking for the quick side effect of the return value to tell us if we have a

    // _blocked_ reader.

    bool busy = false;

    {

        auto lock = std::lock_guard{lock_};

        busy = !Prune(id, 1, uid);

    }

            // It is still busy, blocked reader(s), lets kill them all!

            // otherwise, lets be a good citizen and preserve the slow

            // readers and let the clear run (below) deal with determining

            // if we are still blocked and return an error code to caller.

    // It is still busy, disconnect all readers.

    if (busy) {

        auto reader_threads_lock = std::lock_guard{reader_list_->reader_threads_lock()};

        for (const auto& reader_thread : reader_list_->reader_threads()) {
@@ -240,18 +242,8 @@ bool SimpleLogBuffer::Clear(log_id_t id, uid_t uid) { 
            }

        }

    }

        }

        {

    auto lock = std::lock_guard{lock_};

            busy = Prune(id, ULONG_MAX, uid);

        }



        if (!busy || !--retry) {

            break;

        }

        sleep(1);  // Let reader(s) catch up after notification

    }

    return busy;

    return Prune(id, ULONG_MAX, uid);

}



// get the total space allocated to "id"
@@ -313,16 +305,16 @@ bool SimpleLogBuffer::Prune(log_id_t id, unsigned long prune_rows, uid_t caller_ 


        if (oldest && oldest->start() <= element.sequence()) {

            KickReader(oldest, id, prune_rows);

            return true;

            return false;

        }



        stats_->Subtract(element.ToLogStatisticsElement());

        it = Erase(it);

        if (--prune_rows == 0) {

            return false;

            return true;

        }

    }

    return false;

    return true;

}



std::list<LogBufferElement>::iterator SimpleLogBuffer::Erase(