[asterisk-dev] [Code Review] parkinglots with different parking extensions

Tue Dec 15 03:01:59 CST 2009

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/417/#review1298
-----------------------------------------------------------


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2961>

    Since this holds an extension, I would use AST_MAX_EXTENSION.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2960>

    This debug code should be removed.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2962>

    Please remove this debug function, too.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2963>

    There should be a space before the '{'.  Watch out for formatting guidelines issues.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2964>

    This change is not really related to your patch.  However, if you're going to change it, make it use ast_debug() instead of checking option_debug directly.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2965>

    missing space


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2966>

    missing space. (I'm going to stop marking these.  Check for other instances.)


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2967>

    This added comment does not seem necessary.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2968>

    This comment does not seem necessary.  Also, it also does not appear correct.  park_space_reserve() does not create a parking lot.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2969>

    Add spaces after commas.  Also, please remove the comment.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2970>

    Use ast_debug()


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2971>

    Instead of doing this, just change the declaration to:
    
    char xferto[256] = "";


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2972>

    I would remove this comment.  It's not always accurate since '#' is not always used for transfer (it is configurable), and this transfer function does not always result in parking.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2973>

    This comment can be misleading.  The 'peer' may not be the one being parked.  I would just remove the comment.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2974>

    Why move the comment to be on the same line now?  Be careful of making unnecessary changes.  It makes the patch more difficult to review.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2975>

    I would remove this comment.  It doesn't really provide more information than just reading the line of code.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2976>

    You are missing an ao2_iterator_destroy() here.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2977>

    missing ao2_iterator_destroy()


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2978>

    What is the purpose of changing how this function operates?


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2979>

    remove commented out code


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2980>

    unnecessary comment


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2981>

    missing ao2_iterator_destroy()


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2982>

    You can simplify this down to a single snprintf().


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2983>

    Are there defines anywhere in this file you can use instead of these hardcoded default strings?


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2984>

    This is missing an ao2_iterator_destroy().  However, it looks like you're fixing a bug in parking lot configuration handling that is not necessarily related to your new feature.  I think we should address this configuration handling bug in a separate patch since there is another method I would prefer to use to ensure that parking lots that are no longer in configuration get removed from memory.


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2986>

    looks like you added a space to this line


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2985>

    Remove this comment


/trunk/main/features.c
<https://reviewboard.asterisk.org/r/417/#comment2987>

    Did you remove this code on purpose?


- Russell


On 2009-10-29 09:45:22, mnick wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/417/
> -----------------------------------------------------------
> 
> (Updated 2009-10-29 09:45:22)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> In features.conf you can define one parking extension: for example
> [general]
> parkext = 700
> parkpos = 701-750
> ...
> 
> you can also define new parkinglots like:
> [parkinglot_park2]
> context = park2
> parkpos = 801 - 850
> 
> but it is not possible to set a new 'parkext' to 'parkinglot_park2' (like: parkext = 800)
> 
> 
> Now it should be possible to have more than one parking extension. 
> [parkinglot_park2]
> parkext = 800
> parkpos = 801 - 850
> context = park2
> 
> [parkinglot_sales]
> parkext = 900
> parkpos = 901 - 950
> context = sales
> 
> 
> [Please ignore the 'my_dump_chan()' and 'my_dump_parkinglot()' functions, they're just for testing]
> 
> 
> This addresses bug 14882.
>     https://issues.asterisk.org/view.php?id=14882
> 
> 
> Diffs
> -----
> 
>   /trunk/main/features.c 226458 
> 
> Diff: https://reviewboard.asterisk.org/r/417/diff
> 
> 
> Testing
> -------
> 
> I have tried various scenario's with different parkinglot's and context's. 
> But I am not sure if I've covered all cases.
> 
> 
> Thanks,
> 
> mnick
> 
>