[Asterisk-code-review] main/cdr: Fixed cdr start overwriting (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/10747 )

Change subject: main/cdr: Fixed cdr start overwriting
......................................................................


Patch Set 2:

(1 comment)

https://gerrit.asterisk.org/#/c/10747/2/main/cdr.c
File main/cdr.c:

https://gerrit.asterisk.org/#/c/10747/2/main/cdr.c@1627
PS2, Line 1627: 	if ((!cdr->start.tv_sec) && (!cdr->start.tv_usec)) {
              : 		cdr->start = ast_tvnow();
              : 	}
> Yes, I was thinking about that too. […]
I don't think any modifications to CDRs are acceptable without understanding the consequences of it and detailing it, even if the code fix is small. It is possible that while this fixes your problem it then breaks another. That's what I was referring to with the state machine and the scenario.

It's not a question of "should the start time be reset?" for your scenario, it's a question of "this explicitly reset it before, so was that on purpose? what was that scenario? was that correct?". The answer may indeed be your fix in the end but without a more concrete understanding I'm not comfortable with the change as it could fix one thing, and break another.

What I'd like to see is a comment before the code stating the scenarios where the start time can already be set, and why that is acceptable.



-- 
To view, visit https://gerrit.asterisk.org/10747
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I921bc04064b6cff1deb2eea56a94d86489561cdc
Gerrit-Change-Number: 10747
Gerrit-PatchSet: 2
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Jenkins2 (1000185)
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Thu, 06 Dec 2018 12:48:03 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20181206/499ed9e0/attachment.html>