On Sat Apr 27 12:47:38 2024 +0000, Jinoh Kang wrote:
(nit) It's more beneficial to check `*object_shm` instead of `lock->id` for previous acquisition, and require the caller to initilaize `object_shm` to NULL instead:
if (*object_shm)...and change the assert above to `assert( !*object_shm || lock->id );`. This has several advantages:
- Easier typing. `{0}` is hard to type:
<kbd>Shift/AltGr</kbd>-<kbd>{</kbd>, <kbd>0</kbd>, <kbd>Shift/AltGr</kbd>-<kbd>}</kbd>. In comparison, `NULL` is <kbd>Shift</kbd>-<kbd>N</kbd><kbd>U</kbd><kbd>L</kbd><kbd>L</kbd>. 2. No constraint on lock's representation. We currently require `(struct object_lock){0}` to be an invalid lock state. Using `object_shm` removes this restriction and allows us to change its fields freely. 3. Minimized heap corruption. Forgetting to zero-initialize `object` causes outright crash or infinite retry in the usual case. In comparison, forgetting to zero-initialize `lock` (`lock->session` in particular) leads to invalid pointer free() which is sometimes hard to debug.
I don't think there's any difference in one case or the other. In both case we access an invalid `object_shm` pointer in `get_thread_session_object`, either because the lock id is uninitialized and != 0, or because the pointer is uninitialized and != NULL, before doing anything with the session pointer.
IMO the lock initial state requirement is the same as initializers for pthread mutexes, and I've added an OBJECT_LOCK_INIT definition to make it more explicit.
The (zeroed) initial lock state is a valid state -unlike a NULL pointer-, and only the lock state decides whether the object pointer is usable. The caller doesn't need to and shouldn't have to check the pointer value.