[asterisk-dev] [Code Review] 2550: res_parking: Dynamic Parking Lots

[Matt Jordan]

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



/team/group/bridge_construction/res/parking/parking_bridge.c
<https://reviewboard.asterisk.org/r/2550/#comment17007>

    You can just ast_assert(self->lot != NULL);
    
    That's clearer than the ternary expression



/team/group/bridge_construction/res/parking/parking_bridge.c
<https://reviewboard.asterisk.org/r/2550/#comment17008>

    Your comment on the review made sense. Your comment in code does not.
    
    You'd be better off replacing the comment in the code with the comment in the review.



/team/group/bridge_construction/res/parking/parking_ui.c
<https://reviewboard.asterisk.org/r/2550/#comment17009>

    Put the dashes on the next line. It makes it easier to see what the CLI output will look like if the lines represent the output.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment16993>

    It actually feels like this is a valid user error condition. It's very possible that someone configures a parking lot in features.conf (or some other config file) and fat fingers a dynamic parking lot creation in extensions.conf. If you attempt to create a dynamic parking lot and you've already configured a static one, this should log out a WARNING and return.
    
    Asserts aren't appropriate for code paths that user configuration can go down. Asserts should be used for situations that violate the fundamental design of your application.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment16994>

    Public functions should be prefixed with a namespace. In general, this is 'ast'.
    
    For parking, your global functions should be prefixed with something that notes that they are parking specific, e.g., 'parking_'.
    
    This applies to all of your parking related functions - they should not be global and un-prefixed.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment16995>

    Same finding here about namespaces.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment16998>

    Restructure this to only do the lookup logic one time. You could do something like:
    
    const char *template_name;
    const char *chan_template_name;
    
    ast_channel_lock(chan);
    chan_template_name = ast_strdupa(S_OR(pbx_builtin_getvar_helper(chan, "PARKINGDYNAMIC"), ""));
    ...
    ast_channel_unlock(chan);
    
    if (!ast_strlen_zero(chan_template_name)) {
      template_name = chan_template_name;
    } else {
      template_name = DEFAULT_PARKING_LOT;
    }
    
    template_lot = parking_lot_find_by_name(template_name);
    if (!template_lot) {
      ...
    }
    



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment16999>

    Probably don't need this comment, it's clear that you're cloning the configuration



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment17000>

    Again, you really don't need this comment.
    
    Code comments should explain why you're doing things, not the how or what.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment17002>

    If this also returns NULL, you can get rid of the else.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment17003>

    Don't prescribe solutions in error messages. The error message should inform the user what the error is - for all we know, they don't want to use either channel variable to avoid anything - maybe they just accidentally entered the same parking lot name.
    
    I would phrase it differently, something closer to:
    
    "Failed to create dynamic parking lot %s@%s: parking lot already exists"



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment17004>

    Same finding here



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment17006>

    I dislike the use of the goto here. The overlap checks could be put into their own function call that returns non-zero if the overlap check fails, thus removing the need for the goto.



/team/group/bridge_construction/res/res_parking.c
<https://reviewboard.asterisk.org/r/2550/#comment17005>

    I don't think we need all of the channel variable information in the ERROR message.
    
    1. A DEBUG log will already tell us that.
    2. Most error conditions in constructing the dynamic parking lot already give us specific information. Having multiple error messages when something goes wrong usually adds little value.
    3. In an error message, we need to know what parking lot the channel was attempting to create. Just the lot name is sufficient.
    


- Matt Jordan


On May 17, 2013, 10:13 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2550/
> -----------------------------------------------------------
> 
> (Updated May 17, 2013, 10:13 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21644
>     https://issues.asterisk.org/jira/browse/ASTERISK-21644
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Dynamic Parking Lots. Parking lots that are made based on other parking lots as well as a combination of some variables. They get a little bit of overlap extension checking that they do before actually trying to register any extensions...
> 
> For the most part, they function like normal parking lots. The exception is that they get broken down if they ever become empty. In that sense, they behave as though they have already been disabled, it's just that you can keep parking to them while they are still alive.
> 
> Some of the stuff in this review references functionality that is going to be provided by the extensions registration update once that is finished.
> 
> 
> Diffs
> -----
> 
>   /team/group/bridge_construction/CHANGES 389006 
>   /team/group/bridge_construction/res/parking/parking_applications.c 389006 
>   /team/group/bridge_construction/res/parking/parking_bridge.c 389006 
>   /team/group/bridge_construction/res/parking/parking_ui.c 389006 
>   /team/group/bridge_construction/res/parking/res_parking.h 389006 
>   /team/group/bridge_construction/res/res_parking.c 389006 
> 
> Diff: https://reviewboard.asterisk.org/r/2550/diff/
> 
> 
> Testing
> -------
> 
> Created dynamic parking lots with a number of different variables, made sure I could keep parking additional people to them as long as they didn't break down, made sure they were destroyed when they became empty, made sure they didn't leak memory in any obvious ways (tracked by res_parking.c, parking_bridge.c, and parking_controller.c) made sure they were visible in the UI as long as they were alive.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130520/5da298cb/attachment-0001.htm>