[asterisk-bugs] [Asterisk 0010668]: Incomplete CDR lock

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=10668 
====================================================================== 
Reported By:                arkadia
Assigned To:                murf
====================================================================== 
Project:                    Asterisk
Issue ID:                   10668
Category:                   CDR/General
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     feedback
Asterisk Version:           1.4.11  
SVN Branch (only for SVN checkouts, not tarball releases):  1.4  
SVN Revision (number only!):  
Disclaimer on File?:        N/A 
Request Review:              
====================================================================== 
Date Submitted:             09-07-2007 08:26 CDT
Last Modified:              06-06-2008 02:40 CDT
====================================================================== 
Summary:                    Incomplete CDR lock
Description: 
This is all about the meaning of AST_CDR_FLAG_LOCKED flag for CDR. 
I assume if CDR has this flag set then it wont be updated anymore by any
ast_cdr_ functions except cases when we force such update. ast_cdr_reset()
is example of function which allows to update locked CDRs if special flag
is given.

Based on this assumption I prepared the patch with such changes:

  ast_cdr_setvar() - not allowed to touch locked CDRs. I don't see any
cases when we need to change locked CDRs too. Not touching locked CDRs is
helpfull when ForkCDR being used and every new CDR may have different
custom variables.

  ast_cdr_answer() - don't change status of locked CDRs. That might be
helpfull when we have a chain of CDRs for one incoming call for which we
perform several tries to make outgoing call. Every outgoing attempt may
finish with its own completion code. And if the final attempt is 'answered'
then we have only one CDR with 'answered' status. All other will keep
connection error codes which is very helpful for error monitoring.

 ast_cdr_end() - don't change locked CDRs by the same reason as
ast_cdr_answer()

As there will be no changes in CDR after lock, app_forkcdr.c should be
updated to end original locked CDR.
Also it'll be great to make optional cdr reset in this application, but
thats a theme for another patch.
======================================================================
Relationships       ID      Summary
----------------------------------------------------------------------
related to          0012726 ForkCDR application fails to set duration
====================================================================== 

---------------------------------------------------------------------- 
 arkadia - 06-06-08 02:40  
---------------------------------------------------------------------- 
>Sorry, but all hope of an API consistency argument is gone. The API was
never 
> documented, so for years, everyone made their own mental API based on
its 
> behavior, which forms an 'implicit' documentation. And that unpublished,
but
> ubiquitous mental document based on experimentation, trial, and error is
the
> standard. There is no way you can change the behavior of these funcs and
get
> it merged into the code. Any changes you would like to make to solve
your
> problem must not disturb the current behavior. So, the API must stay as
it >is.

Okay, that may be the way to go too. If everyone agreed on that we keep
AST_CDR_FLAG_LOCKED where is was originally. 
Now can we introduce NEW flag AST_CDR_FLAG_LOCKEDFULL which will be used
together with AST_CDR_FLAG_LOCKED, everywhere where AST_CDR_FLAG_LOCKED is
used and additionally in ast_cdr_answer, ast_cdr_setvar and ast_cdr_end?

Thats the whole changes for cdr.c I'm talking about. I suggest we invite
somebody to comment on these changes.


>> AST_CDR_FLAG_LOCKED is a part of public API used by developers in
their
>> applications. Preferably this flag has constant and clear meaning like:

>> if CDR is marked with this flag it is not updated by any ast_cdr_.... 
>> functions.
 
>Sorry, but this is wrong. AST_CDR_FLAG_LOCKED is NOT part of a public
API.
>It's only minimally documented in the source code. 

I have another point of view: ast_cdr_... functions - are not static
functions and are available for application developers. AST_CDR_FLAG_...
comes together with these functions. Thats why I'm saying it IS a public
API. Luck of documentation doesn't mean it's not an API.

 

>Also, implementing new funcs and apps are definitely out. First of all,
they
> complicate the picture, by adding new but similar apps to the existing
>interface. Users will be confused about which to use in new
implementations.
> Next, new apps are an enhancement, and are not on the table for
discussion >for 1.4; If you want new stuff in trunk, we can talk, but if
you want this in >1.4, then we have to do it this way.

I'm not interested in any applications changes in that bug discussion. I'm
talking about applications only to show the way how ast_cdr_... API can be
used. Lets finish with cdr.c changes first. All applications can be
discussed separately in new bug reports.

>So, given these restrictions, I ask again, will the proposed changes give
you
>what you need?

can you show me changes you propose to do on cdr.c. It may be a dirty
draft, not a tested patch if we want to save time. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
06-06-08 02:40  arkadia        Note Added: 0087881                          
======================================================================