[Asterisk-code-review] astobj2: Header macro's should deal with error's from ao2 ref. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: astobj2: Header macro's should deal with error's from ao2_ref.
......................................................................


Patch Set 2:

> > > > (1 comment)
 > > > >
 > > > > I'm not sure how beneficial this patch is.  Passing bogus ao2
 > > > > objects to these macros/functions is already a programming
 > > error
 > > > > that is caught by assertions if they are enabled.
 > > >
 > > > The idea is that when we've identified a bogus ao2, we should
 > > stop
 > > > passing it around - limit the damage.  An example is in
 > > __ao2_alloc
 > > > I used ao2_t_bump(lockobj, "set lockobj").  The intent was that
 > > if
 > > > lockobj is invalid this would return NULL and abort allocation
 > of
 > > > the new AO2 object.
 > > >
 > > > In any case if you feel this is wasteful / over-aggressive
 > error
 > > > checking I can abandon.
 > >
 > > I think a more useful patch would be to make log_bad_ao2() call
 > > ast_log_backtrace() so the invalid object reference will get a
 > > backtrace in the log.
 > 
 > I can see that but then an invalid object will produce multiple log
 > entries (backtrace) per call to ao2 core function.  Wouldn't that
 > make it more useful to limit the spread of the invalid object
 > pointer (don't spam the logger)?

I don't care about spamming the log because of a programming error.  The programming error is a bug that needs to be fixed.  I do care about unnecessarily spamming the log by working code.

The example case you point out above results in an inaccurate error message by __ao2_alloc() because you passed in a bogus ao2 pointer and not a NULL lockobj.

ao2_bump() and ao2_replace() are macros that duplicate this error checking code everywhere for a case that shouldn't happen in correct code.

I would prefer that this patch be abandoned in favor of log_bad_ao2() generating a backtrace like a failed assert does.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e06d27fb6667f54c960023aa3b93d95b07147de
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No