[Asterisk-code-review] main/cdr.c: Alleviate CDR deadlock (asterisk[15])

[Matthew Fredrickson]

Matthew Fredrickson has posted comments on this change. ( https://gerrit.asterisk.org/9270 )

Change subject: main/cdr.c: Alleviate CDR deadlock
......................................................................


Patch Set 3:

(9 comments)

https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c
File main/cdr.c:

https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3787
PS3, Line 3787: 	int nextms = 0;
> No need to initialize nextms.  You always initialize it below.
Done.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3796
PS3, Line 3796: 	/* manually reschedule from this point in time */
> Comment isn't quite right anymore.  How about:
Done.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3799
PS3, Line 3799: 	if (!nextms) {
              : 		/* I hate doing this, but previously if you set the batch time to 0, it would continuously
              : 		   resubmit batches to the scheduler.  IMHO that seems buggy, but I'd rather not break it
              : 		   if someone was relying on that behavior */
              : 		nextms = 1;
              : 	}
> I'd say it is buggy.  All they are doing is needlessly loading their CPU wh
Ok, I'll drop this code then and fix it in parse_config()


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3807
PS3, Line 3807: 	/* returning zero so the scheduler does not automatically reschedule */
> This comment is not needed anymore as you never return zero now.
Done.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3884
PS3, Line 3884: 	/* Don't call start_batch with cdr_batch_lock held */
> s/call start_batch/submit a batch/
Done


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3899
PS3, Line 3899: 		/* this only can happen if someone sets there batch time to 0 or does 'cdr submit' from the CLI back to back quickly */
> Restore the previous comment as there will normally not be an empty schedul
Done.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3910
PS3, Line 3910: 		ast_debug(2, "Processed %d scheduled CDR batches from the run queue\n", numevents);
> This debug msg isn't quite right.
Not part of my original diff, but I can fix regardless.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@4410
PS3, Line 4410: 	SCOPED_LOCK(batch, &cdr_batch_lock, ast_mutex_lock, ast_mutex_unlock);
> This scoped lock can cause a deadlock when you call start_batch_mode().
I agree.  Removed the scoped lock.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@4424
PS3, Line 4424: 	/* Kill the currently scheduled item */
> This comment needs to be removed.
Done.



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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I6333e865db7c593c102c2fd948cecdb96481974d
Gerrit-Change-Number: 9270
Gerrit-PatchSet: 3
Gerrit-Owner: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 09:05:53 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180627/e42a4da0/attachment-0001.html>