[Coverity] Fix coverity issues #2526
Conversation
djeong20
requested review from
myungjoo,
jijoongmoon,
again4you,
jaeyun-jung,
leemgs,
wooksong,
helloahn,
kparichay,
gichan-jang,
anyj0527,
zhoonit,
lhs8928,
songgot,
jihochu,
DonghakPark,
SeoHyungjun,
baek2sm,
skykongkong8,
EunjuYang and
a team
as code owners
|
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2526. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
| num_observed++; | ||
| if (num_observed == iteration.batch()) { | ||
| notify_lock_guard.unlock(); |
There was a problem hiding this comment.
Why unlock here? It seems that num_observed may experience synchronization error.
After this patch.
Thread 1 Thread 2
LINE 198 (after ++, num_observed=iteration-batch)
LINE 200
LINE 201
LINE 198 (after ++, num_observed = i-b + 1)
LINE 205
LINE 202
LINE 203 (num_objserved = 0)
LINE 205
Before this patch, it becomes num_observed = 1
You can miss num_observed counting after this commit.
There was a problem hiding this comment.
If markFilled() cannot affect num_observed or iteration.batch() and num_observed cannot affect markFilled(), you can do num_observed = 0; before unlock.
There was a problem hiding this comment.
makes sense. how about adding a cv?
std::unique_lock notify_lock_guard(notify_mutex);
notify_emptied_num_obs_cv.wait(
notify_mutex, [this] { return num_observed != iteration.batch(); });
num_observed++;
if (num_observed == iteration.batch()) {
notify_lock_guard.unlock();
iq->markFilled(this);
notify_lock_guard.lock();
num_observed = 0;
notify_num_emptied_cv.notify_all();
}
There was a problem hiding this comment.
It's over-engineering. You don't need that much complexity. (is CV really required? can you do it only with mutex?)
There was a problem hiding this comment.
gotcha. Thank you for your guidance!
This PR resolves coverity issues of overflow, use of auto that causes a copy, missing lock and thread lock. **Self-evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghyeon Jeong <dhyeon.jeong@samsung.com>
djeong20
force-pushed
the
fix/coverity/032924
branch
from
62b6c02
to
12c6fa0
Compare
This PR resolves coverity issues of overflow, use of auto that causes a copy, missing lock and thread lock.
Self-evaluation: