[Asterisk-code-review] Astobj2: Improve error logging from INTERNAL OBJ. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: Astobj2: Improve error logging from INTERNAL_OBJ.
......................................................................


Patch Set 4: Code-Review-1

(8 comments)

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

Line 134: 		struct astobj2 *p ## __LINE__ = NULL; \
The initialization is unnecessary.  p will always be set to something.

What is unfortunate is the assignment to p is hidden inside __INTERNAL_OBJ().  The IS_AO2_MAGIC_BAD() check inside __INTERNAL_OBJ() is not always needed by the users.  log_bad_ao2() doesn't need the check.  I think __INTERNAL_OBJ() needs to be simplified.  __INTERNAL_OBJ() should be the exact opposite of EXTERNAL_OBJ() and return a pointer to the internal object.  If the check for IS_AO2_MAGIC_BAD() is needed the code should explicitly test with it.


Line 363: 	struct astobj2 *obj = INTERNAL_OBJ(user_data, __FILE__, __LINE__, __PRETTY_FUNCTION__);
obj is not tested for NULL like __ao2_trylock() and others.


Line 440: 				user_data, delta, ast_get_tid(), file, line, func, tag ? tag : "");
tag ?: ""


Line 770: 		struct astobj2 *weakproxy_internal = INTERNAL_OBJ(weakproxy, file, line, func);
This can just be __INTERNAL_OBJ().  The validity checks aren't needed since we just created the object.


https://gerrit.asterisk.org/#/c/140/4/main/astobj2_container.c
File main/astobj2_container.c:

Line 787: 		if (ref_debug) {
        : 			__ao2_ref(clone, -1, tag, file, line, func);
        : 		} else {
        : 			ao2_t_ref(clone, -1, "Clone creation failed.");
        : 		}
I think this can be simplified to just:
__ao2_ref(clone, -1, tag ?: "Clone creation failed", file, line, func)


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

Line 1119: 	const char *tag, const char *file, int line, const char *func, int ref_debug)
ref_debug is no longer needed


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

Line 2077: 	const char *tag, const char *file, int line, const char *func, int ref_debug)
ref_debug is no longer needed.  Since this is a vtable callback it isn't needed there either.

Let the ripple effect come into play! :)


https://gerrit.asterisk.org/#/c/140/4/main/format_cap.c
File main/format_cap.c:

Line 124: 	cap = __ao2_alloc(sizeof(*cap), format_cap_destroy, AO2_ALLOC_OPT_LOCK_NOLOCK, S_OR(tag, "ast_format_cap_alloc"), file, line, func);
wrap long line


-- 
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: 4
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: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes