----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3607/#review12179 -----------------------------------------------------------
Thanks for the patch. Most of the recommendations I have are small coding guidelines-related quibbles. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22218> Coding guidelines: Place a space after the if. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22219> I recommend extracting everything in this if block into its own function. It will help to keep the already large reload_queue_rules function from doubling in size. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22220> Coding guidelines: Add a space after the if. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22221> Coding guidelines: This line is exceedingly long. I suggest separating the declarations of these variables onto separate lines. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22222> Coding guidelines: Place a space after the comma. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22223> Allocation failure messages aren't necessary since ast_calloc() already will print out an error. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22225> atoi() does not detect errors (such as the input not being an integer) and will just return 0 on bad values. You can change this to: if (!timestr || sscanf(timestr, "%30d", &penaltychangetime) != 1) This will perform a more type-safe method of getting the value. The %30 in the format specifier is there as (an admittedly arbitrary) a way of preventing input of overly large numbers. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22226> While log messages do not need to be super formal, avoid overly informal or slangy words like "gonna". Also, I would suggest printing timestr in this message so that it is more obvious to the user what failed to parse. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22224> Another case where the error messages isn't needed. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22227> Coding Guidelines: This line is exceptionally long and should be broken up into multiple lines. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22229> Use sscanf instead of atoi. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22228> Coding Guidelines: This line should be broken into two lines. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22230> Use sscanf instead of atoi. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22231> Coding Guidelines: Add a space after the if. trunk/apps/app_queue.c <https://reviewboard.asterisk.org/r/3607/#comment22232> This line can be deleted. - Mark Michelson On June 11, 2014, 10:04 a.m., Michael K. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3607/ > ----------------------------------------------------------- > > (Updated June 11, 2014, 10:04 a.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-23823 > https://issues.asterisk.org/jira/browse/ASTERISK-23823 > > > Repository: Asterisk > > > Description > ------- > > This patch gives the ability (optional) to keep queuerules in realtime > (important to notice that rules would be loaded from both realtime and file > too) > To use add to queuerules.conf general section with option to turn the > relatime on (note that after patch there is no option to call rule "general" > as it is reserved for queuerules settings): > > [general] > realtime_rules = yes > > and add the realtime setting to extconfig.conf, for example in my case it's > mysql and it looks like(will attach the .sql with create for table): > > queue_rules => mysql,asterisk,queue_rules > > in table as you can see you need to specify rule name and as in regular rules > you can use relative setings for min and max (with "-" and "+", e.g. "+100") > if one of the rule parameters is wrong it would be ignored, basically > validation is like from file. > here is the the example of table context as an example: > > rule_name, time, min_penalty, max_penalty > 'default', '10', '20', '30' > 'test2', '20', '30', '55' > 'test2', '25', '-11', '+1111' > 'test2', '400', '112', '333' > 'test3', '0', '4564', '46546' > 'test_rule', '40', '15', '50' > > which would result in : > > Rule: default > After 10 seconds, adjust QUEUE_MAX_PENALTY to 30 and adjust > QUEUE_MIN_PENALTY to 20 > Rule: test2 > After 20 seconds, adjust QUEUE_MAX_PENALTY to 55 and adjust > QUEUE_MIN_PENALTY to 30 > After 25 seconds, adjust QUEUE_MAX_PENALTY by 1111 and adjust > QUEUE_MIN_PENALTY by -11 > After 400 seconds, adjust QUEUE_MAX_PENALTY to 333 and adjust > QUEUE_MIN_PENALTY to 112 > Rule: test3 > After 0 seconds, adjust QUEUE_MAX_PENALTY to 46546 and adjust > QUEUE_MIN_PENALTY to 4564 > Rule: test_rule > After 40 seconds, adjust QUEUE_MAX_PENALTY to 50 and adjust > QUEUE_MIN_PENALTY to 15 > > > If you use realtime, the rules will be always reloaded (no cache option > here), in other words it would delete all rules and re-add them from realtime > and file. It's important to understand that with realtime on it would reload > rules from file doesn't matter if file was or was not change. > While with the option off it would act exactly like it was before patch (file > would not be reloaded if it was not changed) > > > Diffs > ----- > > trunk/apps/app_queue.c 415442 > > Diff: https://reviewboard.asterisk.org/r/3607/diff/ > > > Testing > ------- > > Currently the tests were made on development machine. > > > Thanks, > > Michael K. > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
