Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OS-6997 bhyve should error check mutexes #217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joyent-automation
Copy link

OS-6997 bhyve should error check mutexes

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/4139/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@mgerdts commented at 2018-06-05T04:44:02

Patch Set 1: Code-Review-1

Quick untested hack

@mgerdts commented at 2018-06-05T12:48:25

Uploaded patch set 3: Commit message was updated.

@mgerdts commented at 2018-06-05T15:00:24

Patch Set 4: -Code-Review

I'm not sure if this is a great idea or not, but would like feedback.

When mutexes are initialized with the static initializers, flag.flag1 (lwp_mutex_t) or __pthread_mutex_flags.__pthread_mutex_flag1 (pthread_mutex_t) does not get set to (LOCK_INITED). This is the case with PTHREAD_MUTEX_INITIALIZER and PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP. Aside from that, mdb shows PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP and _check_mutex_init() leading to the same results.

To catch issues with future syncs, we may want to add an ASSERT3S() in _check_mutex_lock() to ensure that PTHREAD_MUTEX_ERRORCHECK is set.

@mgerdts commented at 2018-06-05T15:25:19

Patch Set 4:

Summary of comments at https://chat.joyent.us/joyent/pl/q1r4j3atwpr55cnjc4i6z155wy

  • No need to edit all the files, just use -include CFLAG
  • Use mutex_enter() and mutex_exit()
  • Use mutex_init(&mtx, LOCK_ERRORCHECK, NULL)
  • Consider adding a pthread version of mutex_*() to avoid casts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants