[Home]

Summary:ASTERISK-05806: [patch] new RAND() function
Reporter:Clod Patry (junky)Labels:
Date Opened:2005-12-08 19:14:35.000-0600Date Closed:2006-01-09 17:56:31.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Functions/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) func_rand.c
( 1) func_random.c.20060101
( 2) func_random.c.20060105
( 3) M5961.txt
Description:right now, if u need to generate any random number, you need to launch a separate process to do it.

This is a first try to implement a random function in asterisk.

polux*CLI>
 -= Info about function 'RAND' =-

[Syntax]
RAND(min,max)

[Synopsis]
Choose a random number in a range

[Description]
Choose a random number between bornes.
Examples: exten => 73,1,Set(junky=${RAND(1,8)});
Will pickup a random number in [1,8], bornes are include.



exten => 73,1,Set(bb=${RAND(1,5)});

will produce a random number [0,5] each time:

   -- Executing Set("SIP/10-947a", "bb=4") in new stack
 == Auto fallthrough, channel 'SIP/10-947a' status is 'UNKNOWN'
   -- Executing Set("SIP/10-0970", "bb=3") in new stack
 == Auto fallthrough, channel 'SIP/10-0970' status is 'UNKNOWN'
   -- Executing Set("SIP/10-b2fe", "bb=2") in new stack
 == Auto fallthrough, channel 'SIP/10-b2fe' status is 'UNKNOWN'
   -- Executing Set("SIP/10-7152", "bb=2") in new stack
 == Auto fallthrough, channel 'SIP/10-7152' status is 'UNKNOWN'

Comments:By: Tilghman Lesher (tilghman) 2005-12-09 10:44:52.000-0600

1)  It's preferable if you keep function source within the funcs/ subdirectory.  I know app_cut had both an app and a func, but that was to prevent duplicate code.

2)  Bug ASTERISK-5757 creates an API call ast_random() which would be preferable to utilize, since random() is not thread-safe on certain platforms (e.g. FreeBSD).

3)  As noted in the coding guidelines, you should dispose of exceptional conditions before the main body of code, not afterwards in an else condition.

4)  The indentation is inconsistent.  In certain places, you're indenting with spaces, rather than with tabs.

5)  I don't know why you're doing an snprintf to a variable passed to ast_log.  ast_log() can already do those conversions internally.

6)  Nor do I understand why you're doing an strlen() on buffer, when you've previously memset buffer to be all 0's.

7)  Why use res at all, when you can just snprintf() directly to buffer?
By: Clod Patry (junky) 2005-12-31 09:04:03.000-0600

Changes in that Version:
wrote a separate file that you can [un]load func_random.so.

exceptional conditions before the main body of code, not in the else block.

consistent indentation (tab)

i snprintf directly to buffer, so no more res and the strlen on buffer.

So in fact, everything corydon requested is in that new version, except his point 2, since itsn't in the trunk, im not using it so far.

Cory: i wanna say a huge thanks to you for your comments it's has been really useful, so thanks for taking your time for my code.
By: Tilghman Lesher (tilghman) 2006-01-01 11:13:54.000-0600

1)  Line 54: there's no need to memset the entire buffer, as when you don't have an error, you ALWAYS snprintf() to the buffer.  For dealing with an error, the only thing you need to do is to set the initial character to '\0'.

2)  Line 61: there's no need to ast_strdupa(), as the function that calls it (ast_func_read) already passes in an ast_strdupa'ed buffer.

3)  Line 67: you have a possible buffer overflow, as you called ast_app_separate_args asking for 3 arguments, when you only have 2 to parse (and more importantly, only space in your array for 2).

4)  Line 71: it's good practice to default the min and max to 0 and RAND_MAX, respectively.

5)  Lines 61,74,75: there's no need to cast your arguments, as they are already the specified type.

6)  Line 78: you declare a variable at a location other than at the top of a block.  While this is fine for gcc, it is not ANSI C, and is contrary to the coding guidelines.  Simply switching lines 77 and 78 would be sufficient to correct this.

7)  Line 82: please add spacing to your expression, as per the coding guidelines.

8)  Lines 85,88,91:  please define constants to return here.  It makes the code easier to read, since the error you're returning is expressed as an English phrase.  (Actually, you could get rid of these entirely.  See point 11.)

9)  Line 90:  you're using sizeof(buffer), which is 4 (it's a pointer).  You should have used buflen (a parameter).

10)  Again, random() is not thread-safe on non-Linux architectures.  This desperately needs a centralized routine with a mutex for FreeBSD and other non-Linux systems.

11)  Since this is just a function, not a function and an application, there's actually no need to go with an internal function.  Just inline the code.  (There would be a reason to do this if you needed to call the same code for both a dialplan function, as well as some other API, such as AGI, manager, or CLI, but as it's referenced from only one place, there's no reason not to inline the code here).

12)  Inclusive with this patch, we may want to deprecate the Random application, as what it does can now be accomplished with the combination of this dialplan function, an expression, and GotoIf.
By: Clod Patry (junky) 2006-01-01 14:48:45.000-0600

Points fixed:
1
2,11 (merge everything in the same function)
3, ast_app_separate_args() no longer exists.
4, 0 and RAND_MAX used (if no args)
 0 used for min (if no min)
 RAND_MAX used for max (if no max)
5
6
7
8 no longer any defined here, since everything is in one function
9
12

So everything is here, except your point 11 aboit ast_random(), anyways, that will be really easy to change.
By: Tilghman Lesher (tilghman) 2006-01-02 10:39:10.000-0600

Lines 65-68 are a really bad way of doing what you're trying to do.  Essentially you're allocating space on the stack for a string, assigning a variable that lasts past that stack frame to that stack space.  While it may, in fact, work, it is not good programming practice.

I would get rid of that and do later:
if (!max || sscanf(max, "%d", &max_int) != 1) {
   max_int = RAND_MAX;
}
if (!min || sscanf(min, "%d", &min_int) != 1) {
   min_int = 0;
}
and save yourself the trouble of printing into a space you're going to sscanf later anyway.  This also takes care of the problem of min or max being set to the empty string, which should imply the default min or max.

Also, you really didn't need to get rid of ast_app_separate_args, as that is now the preferred syntax.  I was just pointing out that you had called it improperly.
By: Clod Patry (junky) 2006-01-05 19:11:47.000-0600

your first request is done.

But for your second, itsnt done, cause, the original ast_app_separate_args was in the internal function, remember?

Any more job here to commit?
By: Tilghman Lesher (tilghman) 2006-01-05 22:14:14.000-0600

Here's what I'm ready to commit.  Still needs to be tested, though, which is something I'll do in the morning.
By: Tilghman Lesher (tilghman) 2006-01-09 17:56:31.000-0600

Committed to trunk