[asterisk-dev] Re: [asterisk-commits] murf: branch 1.4 r59486 - in /branches/1.4: include/asterisk/ main/ res/

[Steve Murphy]

On Fri, 2007-03-30 at 09:28 -0500, Kevin P. Fleming wrote:
> > +/*! \brief Discard and free a CDR record 
> > + * \param cdr ast_cdr structure to free
> > + * Returns nothing important -- same as free, but no checks or complaints
> > + */
> > +void ast_cdr_discard(struct ast_cdr *cdr);
> > +
> 
> The function is 'void' it returns nothing at all, not 'nothing important'.

Hmmm. my cleanup wasn't as clean as it could be. This was via copy and
paste.
there are 6 other places in include/asterisk/cdr.h that say the same
thing. I
removed the 'important' from them all.

> 
> > +
> > +	/* Experiment: under what conditions do we NOT want to track cdrs on channels? */
> > +	tmp->cdr = ast_cdr_alloc();
> > +	ast_cdr_init(tmp->cdr, tmp);
> > +	ast_cdr_start(tmp->cdr);
> 
> It this 'experiment' comment just a left-over?

No, really a reminder to me for future work;  there will be places where
CDRs get posted, where they didn't before. I changed Experiment: to
Reminder for future:

> 
> > -	chan->cdr = ast_cdr_alloc();   /* allocate a cdr for the channel */
> > +	if (!chan->cdr) {
> > +		chan->cdr = ast_cdr_alloc();   /* allocate a cdr for the channel */
> > +		ast_log(LOG_NOTICE, "=====PBX_OUTGOING_CDR_FAILED ALLOCS CHANNEL CDR for %s\n", chan->name);
> > +	}
> 
> This message is both incorrect and improperly set to LOG_NOTICE. In fact
> the code doesn't even make any sense, because it just tries twice to
> allocate a CDR... if it failed the first time, why would it succeed the
> second time?

Note that the ast_cdr_alloc() call was removed.  Since the channel_alloc
func allocates a cdr,  I just got rid of the entire if statement
instead. Below this, 
there's a check for a null cdr struct, so all should be OK.

> 
> > +	} else {
> > +		/* make up a new cdr */
> > +		ast_cdr_init(bridge_object.cdr,chan); /* eh, just pick one of them */
> > +		chan->cdr = ast_cdr_alloc();
> > +		ast_cdr_init(chan->cdr, chan);
> > +		peer->cdr = ast_cdr_alloc();
> > +		ast_cdr_init(peer->cdr, peer);
> > +		ast_cdr_start(peer->cdr); /* now is the time to start */
> 
> You cannot blindly assume ast_cdr_alloc() will succeed, since it tries
> to allocate memory. Every place you call it, you need to be prepared for
> it to fail.

Good point. I modified the cdr_alloc() func to at least print out an
ERROR log message if an allocation fails, and try to wrap all the
immediate places where a crash might ensue.

While I don't think that having a CDR failure should terminate the call,
or fail the bridge, this correction was the one that took the most time.
I went back and bullet-proofed all the cdr functions to just return if
the cdr is null. This included ast_cdr_dup(), ast_cdr_getvar(),
ast_cdr_setvar(), check_post(), check_start(), set_one_cid(), and
ast_cdr_detach(); luckily, most of the others loop on the cdr arg, and
if it's null, won't be affected.

> 
> >  			ast_log(LOG_WARNING, "Bridge failed on channels %s and %s\n", chan->name, peer->name);
> > +			/* whoa!! don't go running off without cleaning up your mess! */
> > +			ast_cdr_merge(bridge_object.cdr,chan->cdr);
> > +			ast_cdr_merge(bridge_object.cdr,peer->cdr);
> > +			ast_cdr_failed(bridge_object.cdr);
> > +			ast_cdr_end(bridge_object.cdr);
> > +			ast_cdr_detach(bridge_object.cdr);
> > +			bridge_object.cdr = NULL;
> > +			ast_cdr_free(chan->cdr); /* no posting these guys */
> > +			ast_cdr_free(peer->cdr);
> > +			chan->cdr = NULL;
> > +			peer->cdr = NULL;
> 
> Since the only content of 'bridge_object' is a CDR, how about just not
> creating a new structure at all, and instead just declaring a
> 'bridge_cdr' variable in this function to hold the CDR?

Good advice. I did exactly that. When I started, I wasn't sure if the
cdr was all I'd need, but it was, so the bridge object isn't necc. to
glue things together.

> 
> > +#ifdef NOT_NECC
> > +		ast_log(LOG_NOTICE,"Channel name is %s, and the cdr channel name is '%s'\n", chan->name, chan->cdr->channel);
> > +		if (!ast_strlen_zero(chan->name) && ast_strlen_zero(chan->cdr->channel)) {
> > +			ast_copy_string(chan->cdr->channel, chan->name, sizeof(chan->cdr->channel));
> > +		}
> > +#endif
> 
> What is this NOT_NECC bit?

It was indeed not neccessary, so I removed it.

Many thanks, Kevin, for a great review! See 59522 (1.4) and 59523
(trunk).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3239 bytes
Desc: not available
Url : http://lists.digium.com/pipermail/asterisk-dev/attachments/20070330/f24513bd/smime.bin