[Asterisk-code-review] vector: Traversal, retrieval, insert and locking enhancements (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: vector:  Traversal, retrieval, insert and locking enhancements
......................................................................


Patch Set 5:

(2 comments)

https://gerrit.asterisk.org/#/c/339/5/include/asterisk/vector.h
File include/asterisk/vector.h:

Line 55: #define AST_RWVECTOR(name, type) \
I don't have much problem with this definition.  The only reason to have a lock as part of the struct is so you can pass a pointer to it and create your own manipulation routines.


The following are all you would need for a version with a RW lock:
AST_VECTOR_RW()
AST_VECTOR_RW_INIT()
AST_VECTOR_RW_FREE()
AST_VECTOR_RW_RDLOCK()
AST_VECTOR_RW_WRLOCK()
AST_VECTOR_RW_UNLOCK()
also bonus:
AST_VECTOR_RW_RDLOCK_TRY()
AST_VECTOR_RW_WRLOCK_TRY()
AST_VECTOR_RW_RDLOCK_TIMED()
AST_VECTOR_RW_WRLOCK_TIMED()

The the other macros you've defined are only for convenience.  Manipulating the lock within the macros precludes them from being used as part of an atomic multi-part vector manipulation.  This is especially true for RW locks since they are not re-entrant.


Line 457: /*!
        :  * \brief Get an address of element in a vector.
        :  *
        :  * \param vec Vector to query.
        :  * \param idx Index of the element to get address of.
        :  */
        : #define AST_VECTOR_GET_ADDR(vec, idx) ({	\
        : 	size_t __idx = (idx);			\
        : 	ast_assert(__idx < (vec)->current);	\
        : 	&(vec)->elems[__idx];			\
        : })
        : 
        : #define AST_RWVECTOR_GET_ADDR(vec, idx) ({ \
        : 	typeof((vec)->elems[0]) res = (typeof((vec)->elems[0]))NULL; \
        : 	if (ast_rwlock_rdlock(&(vec)->lock) == 0) { \
        : 		res = AST_VECTOR_GET_ADDR(vec, idx); \
        : 		ast_rwlock_unlock(&(vec)->lock); \
        : 	} \
        : 	res; \
        : })
> The assert could pass but the get could fail if the vector was being modifi
Exactly.  The returned value of AST_RWVECTOR_GET_ADDR() is not reliable since another thread could invalidate what it points to at any time.

This macro can only be used in a multi-part atomic vector operation and you *must* control the locking manually.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2e07ecc709d2f5f91bcab8904e5e9340609b00e0
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes