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

[Corey Farrell]

Corey Farrell has posted comments on this change.

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


Patch Set 6:

(4 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?
Nothing.  TRACE_FRAMES is not used anywhere.  I will split this to another review / apply to all branches.


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

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 implementatio
Probably right, I guess __ao2_traverse isn't called enough to be worth going this far with the optimization.


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.  Ot
Didn't see that 20 is available, will use that.


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_destr
This balances the NULL tag of ao2_container_alloc with a NULL tag in container_destruct.  It is assumed that if you explicitly use a NULL tag for container alloc, you will use NULL tags though-out.

Maybe it would be better to always use the debug version and pass tag ?: "" to __ao2_alloc?


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