[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)

If you look at the linked list macros you see that the locking is handled manually anyway.  The list operations do not automatically lock/unlock the lock and most of the RWLIST defines are simply defines to change the name to the normal list define.

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

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; \
        : })
This vector operation is kind of useless for a locked variant.  The returned address contents could become invalid on the macro's return.

Thread 1:
addr = AST_RWVECTOR_GET_ADDR(vec, 1)
Thread 2:
AST_RWVECTOR_INSERT_AT(vec, 1, elem)
Thread 1:
addr is now invalid as what it points to is not what it used to be.  Also if the insert actually increased the vector then addr could point to freed memory.


Line 521: #define AST_RWVECTOR_GET_CMP(vec, value, cmp) ({ \
Same for this one.  Unless you only care if the element was in the vector or not.


-- 
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