RDKEMW-23770: BLE notification enable reported as successfull under error cases

[egalla204]

If BleGattNotifyPipe fails to create the notification thread, it will still report as Valid() = true. This causes notifications to be erroneously reported as successfully enabled

[Copilot]

Pull request overview

This PR updates the BlueZ BLE GATT notification pipe validity check to ensure the underlying notification thread is running when isValid() is queried, aligning the object’s “valid” state with its ability to actually receive notifications.

Changes:

• Update BleGattNotifyPipe::isValid() to require both an open pipe FD and a running notify thread.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/control/276/rdk/components/generic/control

• Commit: 7a1b3d7

Report detail: gist'

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:75

• NotifyThread() sets m_notifyThread.running=false on exit, but shutdown()/ThreadJoin() currently skip joining when running==false. If the thread exits before shutdown() runs, the join is skipped and the joinable pthread resources are never reclaimed.

Gate joining on whether a thread was created (e.g., id != 0) rather than the live running flag, and clear id after a successful join so repeated shutdowns remain safe.

    if (!thread->running.load()) {
        XLOGD_WARN("Thread <%s> not running.", thread->name);
        return (true);
    }

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:222

• Related to the join logic: shutdown() currently only calls ThreadJoin() when running==true. If the thread has already exited (and cleared running), shutdown will skip joining.

After changing ThreadJoin() to use id as the "created / join needed" indicator, update this check to use m_notifyThread.id != 0 as well.

    if (m_notifyThread.running.load()) {
        if(FD_SIGNAL(m_exitEventFds) > -1) {
            SignalEventFd(FD_SIGNAL(m_exitEventFds));
        }
        ThreadJoin(&m_notifyThread, 2);
    }

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/control/276/rdk/components/generic/control

• Commit: bb10a13

Report detail: gist'

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:74

• ThreadJoin() returns early when running==false, which prevents joining a thread that has already exited. If the notify thread terminates before shutdown() is called (and clears running), the join is skipped and the joinable thread resources can leak. Consider joining based on thread->id (created) rather than running.

This issue also appears on line 216 of the same file.

    if (!thread->running.load()) {
        XLOGD_WARN("Thread <%s> not running.", thread->name);
        return (true);
    }

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:345

• When *isAlive is false (which is expected during normal destruction because the destructor sets it before shutdown()), the thread logs this as an error and suggests something went wrong. This message is likely to be noisy/misleading during normal teardown.

        XLOGD_ERROR("BleGattNotifyPipe object has been destroyed before thread exited.  Suspect something went wrong, exiting...");

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:220

• shutdown() only joins the notify thread when running==true. If the thread has already exited and cleared running before shutdown() runs, shutdown() won't join and the joinable thread resources can leak. Joining based on thread id (created) avoids this.

    if (m_notifyThread.running.load()) {
        if(FD_SIGNAL(m_exitEventFds) > -1) {
            SignalEventFd(FD_SIGNAL(m_exitEventFds));
        }
        ThreadJoin(&m_notifyThread, 2);

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/control/276/rdk/components/generic/control

• Commit: 58350ee

Report detail: gist'

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:56

• ThreadJoin() uses thread->id == 0 as the sentinel for “not running”. If pthread_create() fails, ThreadCreate() currently leaves thread->id unchanged, which can cause a later ThreadJoin() to attempt joining a stale/non-related thread id (e.g., if ThreadCreate() is called again after a previous run). Clearing thread->id before the create attempt and again on failure makes the sentinel reliable.

This issue also appears on line 69 of the same file.

static bool ThreadCreate(BleThread *thread, void *(*start_routine)(void *), void *arg, pthread_attr_t *attr = NULL)
{
    thread->running.store(false);
    if (0 != pthread_create(&thread->id, attr, start_routine, arg)) {
        XLOGD_ERROR("unable to launch thread <%s>", thread->name == NULL ? "" : thread->name);
        return (false);
    }

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:74

• Using thread->id == 0 and assigning thread->id = 0 assumes pthread_t is an integer type with 0 as an invalid value. pthread_t is an opaque type in POSIX, so this sentinel is non-portable and can be invalid on some platforms/toolchains. Prefer tracking “thread created/joinable” with an explicit flag in BleThread (e.g., std::atomic<bool> joinable) rather than encoding state into pthread_t.

static bool ThreadJoin(BleThread *thread, uint32_t timeout_secs)
{
    if (thread->id == 0) {
        XLOGD_WARN("Thread <%s> not running.", thread->name);
        return (true);
    }

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/control/276/rdk/components/generic/control

• Commit: ff6c0b2

Report detail: gist'

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:74

• shutdown() now always calls ThreadJoin(), so the ThreadJoin() early-return path (id == 0) will emit a WARN even in normal/expected cleanup cases (e.g., thread creation failure). This can create misleading log noise; consider downgrading this to DEBUG (or removing the log) since it isn't necessarily abnormal.

    if (thread->id == 0) {
        XLOGD_WARN("Thread <%s> not running.", thread->name);
        return (true);

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/control/276/rdk/components/generic/control

• Commit: ff6c0b2

Report detail: gist'

[rdkcmf-jenkins]

b'## WARNING: A Blackduck scan failure has been waived

A prior failure has been upvoted

• Upvote reason: ok

• Commit: ff6c0b2
  '

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:56

• If pthread_create() fails, ThreadCreate() returns false without resetting thread->id. Since ThreadJoin() now uses id==0 as the "not running" sentinel, leaving a stale/non-zero id here can lead to incorrect join attempts later if ThreadCreate() is ever called on a reused BleThread instance (or if id was not initialized by the caller).

static bool ThreadCreate(BleThread *thread, void *(*start_routine)(void *), void *arg, pthread_attr_t *attr = NULL)
{
    thread->running.store(false);
    if (0 != pthread_create(&thread->id, attr, start_routine, arg)) {
        XLOGD_ERROR("unable to launch thread <%s>", thread->name == NULL ? "" : thread->name);
        return (false);
    }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/ble/hal/blercu/bluez/blegattnotifypipe.cpp:355

• When the owning object is being destroyed (*isAlive == false), the loop exits but running is still true, which causes an XLOGD_ERROR("...exited unexpectedly") even though this is an expected teardown path. This can create misleading error logs during normal destruction/shutdown.

    if (!running) {
        XLOGD_INFO("BLE notification pipe thread exited gracefully.");
    } else {
        XLOGD_ERROR("BLE notification pipe thread exited unexpectedly, suspect an error occurred...");
    }

src/ble/hal/blercu/bluez/blegattnotifypipe.h:47

• pthread_t is an opaque type, so treating id == 0 / id = 0 as a universal “not running” sentinel (as done in ThreadCreate()/ThreadJoin()) is not portable across pthread implementations. Consider adding an explicit std::atomic<bool> joinable/created flag (set true after a successful pthread_create, set false after a successful join) and use that for the fast-path checks instead of comparing pthread_t to 0.

typedef struct {
   const char *      name;
   pthread_t         id;
   std::atomic<bool> running;
} BleThread;