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

[Kevin P. Fleming]

> +/*! \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'.

> +
> +	/* 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?

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

> +	} 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.

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

> +#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?