[Asterisk-code-review] Astobj2: Allow reference debugging to be enabled/disabled by... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: Astobj2: Allow reference debugging to be enabled/disabled by config.
......................................................................


Patch Set 6: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/#/c/140/6/build_tools/cflags-devmode.xml
File build_tools/cflags-devmode.xml:

Line 18
What does this change have to do with ref debugging?


https://gerrit.asterisk.org/#/c/140/6/include/asterisk/astobj2.h
File include/asterisk/astobj2.h:

Line 147: Each of reference manipulation will generate one line of output in in the refs
Each of the reference manipulations...
or
Each reference manipulation...


Line 1552: enum ao2_callback_type {
         : 	AO2_CALLBACK_DEFAULT,
         : 	AO2_CALLBACK_WITH_DATA,
         : };
         : 
         : /*!
         :  * \internal
         :  * \brief Traverse the container.  (internal)
         :  *
         :  * \param self Container to operate upon.
         :  * \param flags search_flags to control traversing the container
         :  * \param cb_fn Comparison callback function.
         :  * \param arg Comparison callback arg parameter.
         :  * \param data Data comparison callback data parameter.
         :  * \param type Type of comparison callback cb_fn.
         :  * \param tag used for debugging.
         :  * \param file Debug file name invoked from
         :  * \param line Debug line invoked from
         :  * \param func Debug function name invoked from
         :  *
         :  * \retval NULL on failure or no matching object found.
         :  *
         :  * \retval object found if OBJ_MULTIPLE is not set in the flags
         :  * parameter.
         :  *
         :  * \retval ao2_iterator pointer if OBJ_MULTIPLE is set in the
         :  * flags parameter.  The iterator must be destroyed with
         :  * ao2_iterator_destroy() when the caller no longer needs it.
         :  */
         : void *__ao2_traverse(struct ao2_container *self, enum search_flags flags,
         : 	void *cb_fn, void *arg, void *data, enum ao2_callback_type type,
         : 	const char *tag, const char *file, int line, const char *func);
You really shouldn't go this far in exposing the internal ao2 implementation.  You lose the callback function type safety.  Just stop at __ao2_callback() and __ao2_callback_data().


https://gerrit.asterisk.org/#/c/140/6/include/asterisk/options.h
File include/asterisk/options.h:

Line 99: 	/*! Reference Debugging */
       : 	AST_OPT_FLAG_REF_DEBUG = (1 << 31),
I think you should use the available bit 20 instead of the sign bit 31.  Otherwise, we may have to consider switching the flags to the 64 bit version.

We might want to re-evaluate these option flags with an eye to eliminating some of them.  (It would need to be another patch of course. :) )


https://gerrit.asterisk.org/#/c/140/6/main/astobj2.c
File main/astobj2.c:

Line 480: 			internal_weakproxy = __INTERNAL_OBJ_CHECK(weakproxy, file, line, func);
This should just be INTERNAL_OBJ_CHECK() as the given file,line,func are for the real object.


https://gerrit.asterisk.org/#/c/140/6/main/astobj2_hash.c
File main/astobj2_hash.c:

Line 1092: 		tag ? container_destruct_debug : container_destruct, ao2_options,
There doesn't seem to be a need for the distinction between container_destruct_debug and container_destruct any more.  Should just be container_destruct and the container_destruct() should pass the ao2_callback tag "container destructor"


https://gerrit.asterisk.org/#/c/140/6/main/astobj2_rbtree.c
File main/astobj2_rbtree.c:

Line 2052: 		tag ? container_destruct_debug : container_destruct, ao2_options,
same here about container_destruct_debug vs container_destruct


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf3552721fe999365ba8a8cf00a965aa6b897cc1
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes