[Asterisk-code-review] lock: Add named lock capability (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: lock:  Add named lock capability
......................................................................


Patch Set 4: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/2521/4//COMMIT_MSG
Commit Message:

Line 13: database. Locking one ao2 object doesn't have any effect on the other.
s/other./other even if those objects had locks in the first place./


https://gerrit.asterisk.org/#/c/2521/4/include/asterisk/lock.h
File include/asterisk/lock.h:

Line 755:  * \defgroup named_locks Named mutex and read-write locks
After looking over the API, I think this isn't a good way to do it.  When using the trylock you will be creating and destroying the named lock for every attempt.  Rather than the lock/trylock creating the named lock you simply have a call to get/put a named lock.  Then you use the normal ao2_lock/ao2_trylock/ao2_unlock functions.

Get or create a named lock
struct ast_named_lock *__ast_named_lock_get(lock_type, keyspace, key, filename, lineno, func)
This function should assert if the requested lock type does not match a pre-existing named lock.

Put back or destroy a named lock.  You can no longer use the lock without getting it back.
void ast_named_lock_put(lock, filename, line, func)

lock = ast_named_lock_get(AST_NAMED_LOCK_MUTEX, keyspace, key)
if !lock return bad news bub
ao2_lock(lock)
...do stuff...
ao2_unlock(lock)

while ao2_trylock(lock) sleep-on-it
...do stuff...
ao2_unlock(lock)

ast_named_lock_put(lock)


Line 764:  * Locking one ao2 object doesn't have any effect on the other.
s/other./other even if those objects had locks in the first place./


https://gerrit.asterisk.org/#/c/2521/4/main/lock.c
File main/lock.c:

Line 1419: 	named_locks = ao2_container_alloc_hash(0, 0, NAMED_LOCKS_BUCKETS, named_locks_hash, NULL, named_locks_cmp);
long line


Line 1434: 	sprintf(concat_key, "%s-%s", keyspace, key);
/* safe */


Line 1439: 		lock = ao2_alloc_options(sizeof(*lock) + strlen(concat_key) + 1, NULL,
You have already calculated the concat_key length when you allocated the buffer space.


PS4, Line 1439: 		lock = ao2_alloc_options(sizeof(*lock) + strlen(concat_key) + 1, NULL,
              : 			lock_how == AST_NAMED_LOCK_REQ_MUTEX ? AO2_ALLOC_OPT_LOCK_MUTEX : AO2_ALLOC_OPT_LOCK_RWLOCK);
check for failure on return


PS4, Line 1518: 	ao2_ref(lock, -1);
              : 	if (ao2_ref(lock, 0) == 1) {
              : 		ao2_unlink_flags(named_locks, lock, OBJ_NOLOCK);
              : 	}
Doing an ao2_ref after unreffing is not a good idea because you may have released the last ref.

You should do it like this:
if (ao2_ref(lock, -1) == 2) {
   ao2_unlink()
}


-- 
To view, visit https://gerrit.asterisk.org/2521
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If258c0b7f92b02d07243ce70e535821a1ea7fb45
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes