On Tue, 2009-04-07 at 12:14 -0400, Damian Krzeminski wrote:
> http://track.sipfoundry.org/browse/XCF-3413
> 
> I made some changes on top of David's patch in the areas concerning 
> sipXconfig.
> 
> Looking for a volunteer to review the non-sipXconfig part and commit it all
> (or letting me know that it works after the review so that I can commit it).

The ZoneAdminRpc.h file should contain a Doxygen description on the
ZoneAdminRpcExec class that provides a detailed description of the XML
RPC method it implements.  See ImdbRpc.h (class ImdbRpcReplaceTable)
for a good example (and
http://sipxecs.sipfoundry.org/doc/sipxsupervisor/class_imdb_rpc_replace_table.html
for the resulting document).

I'd have put the source and installation of sipxzoneadmin and its
setup script into sipXpbx (rather than sipXcommserverLib) so that it's
in the same project with the shell script.  Not a big deal, but I
think it would be less confusing to someone trying to find all the
moving parts.

I like to see the changes to initial-config done differently.  It
should not dynamically determine it's domain name or ip address; if
that information is needed, it should be added as option arguments and
passed in by sipXconfig.  That should be changed for myHostname as
well while we're at it.

Some general shell script advice:

- Don't use 'grep' to do string compares:

     myDomname=`dnsdomainname`
     myZonedns=`host -t NS ${myDomname}`
     echo "$myZonedns" | grep "$myHostname"
     if [ $? -ne 0 ]

  is better written as:

     myDomname=`dnsdomainname`
     myZonedns=`host -t NS ${myDomname}`
     echo "$myZonedns" | grep "$myHostname"
     if [ "${myDomname}" = "${myZonedns}" ]

- Also on the above, use the 'dns_ns' function provided by sipx-utils.sh
  rather than the 'host' command to get the server for the domain.

- When you want to write a file, use a here-document rather than a
  series of separate commands, so this:

    {
       printf "options {\n"
       printf "    directory \"/var/named\";\n"
       printf "    dump-file \"/var/named/data/cacahe_dump.db\";\n"
       printf "    statistics-file \"/var/named/data/named_stats.txt\";\n"
       printf "};\n"
       printf "\n"
       printf "zone \"$myDomname\" IN {\n"
       printf "    type slave;\n"
       printf "    file \"$myDomname.zone\";\n"
       printf "    masters {$myHostaddr;};\n"
       printf "};\n"
    } > "${INITIAL_CONFIG}/etc/named.conf"

  should be:

    cat > "${INITIAL_CONFIG}/etc/named.conf" <<EOF
options {
    directory "/var/named";
    dump-file "/var/named/data/cacahe_dump.db";
    statistics-file "/var/named/data/named_stats.txt";
};

zone "$myDomname" IN {
    type slave;
    file "$myDomname.zone";
    masters {${myHostaddr};};
};
EOF

  the later form is much easier to read (even more so than seen above
  when seen in an editor that does proper syntax-coloring for shell
  scripts).  It also is much easier to get the escaping right, because
  it's almost never needed at all.

- Always put shell variables in ${}:
  Use ${myHostaddr} rather than $myHostaddr even when you don't have
  to; it's just a good habit and will save you some difficult-to-spot
  errors someday.








_______________________________________________
sipx-dev mailing list
[email protected]
List Archive: http://list.sipfoundry.org/archive/sipx-dev
Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev

Reply via email to