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

[Mark Michelson]

Mark Michelson has posted comments on this change.

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


Patch Set 2: Code-Review+1

> > The changes look good to me, but I do have a question about some
 > > patterns I noticed in there.
 > >
 > > There are several places where a "tag" variable is checked for
 > > NULLity. In some cases, it changes what gets passed to another
 > ao2
 > > function to some default string. In other cases, a NULL tag
 > results
 > > in not printing a debug message to the ref_log.
 > >
 > > My problem is, I don't understand how a tag would ever be NULL.
 > In
 > > the case where the non _t_ version of a function is called, the
 > tag
 > > defaults to an empty string, not NULL. Pretty much, someone would
 > > have to call the _t_ variant of an ao2 function and purposely
 > pass
 > > NULL as the debug message. This seems like a mistake, and should
 > > probably be caught with an assertion. It seems like you should be
 > > able to have confidence in a non-NULL tag at all times.
 > 
 > This is intentional.  Previously a call directly to __ao2_ref would
 > never write to refs log, even if REF_DEBUG is enabled.  So any code
 > that previously called __ao2_ref(o, +1) would now call ao2_t_ref(o,
 > +1, NULL) - to maintain the previous behaviour of no entry in refs
 > log.  Probably need to add to the doxygen comment for ao2_ref /
 > ao2_alloc to indicate this behaviour.  I'm busy tonight, I'll try
 > to add a comment in the next couple days.
 > 
 > Container code is a bit weird, the node objects do not get logged
 > by REF_DEBUG.  This was always the case, I'm guessing it's a matter
 > of volume / not flooding the logs.

Okay that makes sense then. Changing to +1.

-- 
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: 2
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-HasComments: No