[asterisk-dev] [Code Review] T.30 <-> T.38 Gateway application

Thu Jan 28 15:02:10 CST 2010

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

I don't know a whole lot about T.38 or spandsp. However, I am very familiar with Asterisk and its coding guidelines, so I have put together an initial review focusing on coding practices more than whether the application actually works correctly.

This probably isn't everything that can be corrected, but I found quite a bit of things that need correcting just from my first glance at the code.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3198>

    As someone not intimately familiar with T.38 gatewaying, the documentation for this parameter seems very strange to me.

    First of all, why would an option called "timeout" control whether the application acts as the caller or receiver?

    Second, what type of argument can be given for this option? The documentation makes it sound like this takes a yes/no or true/false value. Looking at the code, though, it appears that timeout should be a number of seconds...for something that the documentation doesn't explain at all.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3189>

    Anywhere you see these sorts of red blotches in the review means that you have left some sort of trailing whitespace. These all should be removed.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3190>

    Asterisk coding guidelines forbid declaring variables after code statements. Please move these to the top of the function.

    If you run the configure script with --enable-dev-mode, then any warnings you see will be treated as errors, and so you will be more likely to fix warnings.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3219>

    This if statement can be inverted to reduce the level of nesting in this function.

    i.e.

    if (!t38_gateway_init(&t38_state, t38_tx_packet_handler, channels[0]) {
        ast_log(LOG_ERROR, "ERROR MESSAGE\n");
        s->finished = 1;
        return;
    }

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3191>

    Be sure to places spaces before and after binary operators. In this case, the '=' and the '<' operators need spaces before and after them.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3192>

    Why 20?

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3195>

    Place the { on the same line as the if

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3194>

    Spaces before and after '='

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3197>

    This comment makes no sense.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3225>

    Spaces before and after '*'

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3193>

    While getting a NULL frame is a possible indication that a hangup occurred, another valid one is receiving a frame of type AST_FRAME_CONTROL and subclass AST_CONTROL_HANGUP. This needs to be handled.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3196>

    Spaces before and after '='

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3224>

    Please use %d for integers instead of %i.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3212>

    Spaces before and after '='

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3211>

    Use curly braces around the else

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3209>

    Why 20?

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3210>

    What is the point of this? You set cstate[0] and cstate[1] and then immediately overwrite them with different values.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3222>

    It seems a bit strange to me that all frames are written to the inactive channel. For instance, control frames should not be written to the inactive channel.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3214>

    I would recommend restructuring this section to avoid the amount of nesting.

    For instance, you have something like

    if ((cstate[1] != T38_STATE_NEGOTIATED) && (cstate[1] != T38_STATE_NEGOTIATING) {
        /* GIANT BLOCK OF CODE */
    }

    if ((cstate[1] == T38_STATE_NEGOTIATED) || (cstate[1] == T38_STATE_NEGOTIATING) {
        ast_frfree(f);
        continue;
    }
    /* GIANT BLOCK OF CODE */

    This can be done with every if statement in this block.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3215>

    Is it possible for the 'f' and 'e' DTMF to be capitalized as 'F' or 'E' ?

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3213>

    Do not put code statements on the same line as a case label.

    Also spaces before and after '=' on both lines.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3216>

    I don't claim to know much about Asterisk's T.38 architecture, but I seem to recall that the AST_CONTROL_T38 subclass was scrapped because it was not a good model. This is why the control frame type has "XXX" at the beginning of the constant. Unfortunately, I don't know what the recommended fix for this is. Someone else with that knowledge needs to comment.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3223>

    A NULL frame indicates a hangup, but so does a frame of type AST_FRAME_CONTROL and subclass AST_CONTROL_HANGUP.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3217>

    Spaces before and after '='

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3218>

    Use curly braces.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3203>

    Spaces before and after '='.

    Also I would recommend using ast_calloc here to ensure that all fields of s are zeroed out at initialization.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3204>

    Spaces before and after all '='s on this line.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3199>

    Declare variables at the beginning of a scope. In this case, AST_DECLARE_APP_ARGS needs to be moved to the other variable declarations at the beginning of t38gateway_exec().

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3220>

    The fdtimeout option is completely undocumented in the XML.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3200>

    More variable declarations that should be at the top of the function.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3202>

    Please use curly braces around all if, else, for, while, do, and switch statements, even if they are only a single line.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3201>

    Please document this default timeout in the XML documentation at the top of the file.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3206>

    Use curly braces for all these ifs.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3221>

    This should be done before dialing the outbound channel.

    If the request for the outbound channel should take a long time, it is possible that the incoming channel will automatically hang up since it has not yet been answered. I believe it is best that you answer the incoming channel as soon as possible in this function.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3207>

    Use curly braces for all these ifs.

/trunk/apps/app_fax.c
<https://reviewboard.asterisk.org/r/459/#comment3208>

    Curly braces.

- Mark

On 2010-01-28 11:49:06, dafe_von_cetin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/459/
> -----------------------------------------------------------
> 
> (Updated 2010-01-28 11:49:06)
> 
> 
> Review request for Asterisk Developers and lmadsen.
> 
> 
> Summary
> -------
> 
> Hi,
> 
> I'm sending you patch containing new application ("FaxGateway") which is able to mediate T30 to T38 and vice versa.
> Feature is using spandsp library.  (I used spandsp-0.0.6pre2).
> 
> This is my first attempt for review so please if I made a mistake let me know I will try to avoid/fix next time.
> 
> Best regards
> Daniel.
> 
> 
> This addresses bug 13405.
>     https://issues.asterisk.org/view.php?id=13405
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_fax.c 239425 
> 
> Diff: https://reviewboard.asterisk.org/r/459/diff
> 
> 
> Testing
> -------
> 
> Done. Works for many people.
> Please refer to:
> https://issues.asterisk.org/view.php?id=13405
> 
> 
> Thanks,
> 
> dafe_von_cetin
> 
>