Hi Clay,
I'm wondering why you are using external shell scripting to test for multi-homed
in installadm.c, and didn't use a C api like getifaddrs?
Attached is a sample test program to I wrote to test this - ok, it's not
perfect, but I'm just trying to give you the general idea...
Thanks,
Darren.
On 07/14/10 10:09 PM, John Fischer wrote:
> Clay,
>
> Here is my feedback on the original code review.
>
> Thanks,
>
> John
>
>
> -----------------------------------------
> ai_sd.py
> -----------------------------------------
> Shouldn't the Copyright include 2009 as well as 2010? Or the date of the
> original file creation?
>
> dhcpinfo is fine for the Popen() but then reassigning and accessing it gets
> a little confusing from a readability/sustainability standpoint:
>
> 399 dhcpinfo = Popen(['/sbin/dhcpinfo','BootSrvA'],
> 400 stderr=PIPE, stdout=PIPE)
> 401 # get (stdout, stderr) tupple
> 402 dhcpinfo = dhcpinfo.communicate()
> 403 if dhcpinfo[1]:
> 404 AISD_LOG.post(AILog.AI_DBGLVL_ERR,
> 405 "Couldn't find BootSrvA record in DHCP;
> error %s",
> 406 dhcpinfo[1])
> 407 return 2
> 408 # dhcpinfo(1) will return a string akin to '192.168.5.1\n'
> 409 svc_address = dhcpinfo[0].strip()
>
> Perhaps this could be changed to be something like:
>
> dhcpinfo = Popen(['/sbin/dhcpinfo','BootSrvA'],
> stderr=PIPE, stdout=PIPE)
> (dhcpout, dhcperr) = dhcpinfo.communicate()
> if dhcperr:
> AISD_LOG.post(AILog.AI_DBGLVL_ERR,
> "Couldn't find BootSrvA record in DHCP; error %s",
> dhcperr)
> return 2
> # dhcpout will return a string akin to '192.168.5.1\n'
> svc_address = dhcpout.strip()
>
> -----------------------------------------
> manifest-locator
> -----------------------------------------
> When I checked this below I got some weirdness with a $ left in it.
> So I am not 100% it works as expected. Not even sure I pulled out
> the environment variables correctly to check. I am concerned that the
> code is replacing $serverIP with something that might leave in a $ which
> might be unexpected. Unless that is expected. 8-)
>
> 114 # Check to see if AI_SERVICE_ADDRESS has the $serverIP keyword
> 115 #
> 116 if [ -n "$(print "$AI_SERVICE_ADDRESS" | $GREP
> '$serverIP')" ]; then
> 117 AI_SERVICE_ADDRESS=$(print $AI_SERVICE_ADDRESS | \
> 118 $SED "s/\$serverIP/$($DHCPINFO $BOOTSERVER)/")
> 119 fi
>
>
> -----------------------------------------
> installadm-common.sh
> -----------------------------------------
> The comment should probably be more specific about 'this'. Also is
> there a bug
> that is being tracked for the Solaris sed(1) not being happy? I think
> you just
> using the GNU sed's \n feature which Solaris sed(1) does not have.
>
> 40 # XXX this needs to go away but Solaris sed(1) isn't happy
> 41 GNUSED=/usr/gnu/bin/sed
>
> Why assume multihomed if no interfaces are up? Mostly for my FYI.
>
> 459 # see if the machine has more than one interface up/no
> interfaces up
> 460 # or if the hostname is the keyword "$serverIP" we are
> multihomed
> 461 if [[ $(valid_networks | $WC -l) -ne "1" || \
> 462 "$svc_address_hostname" == '$serverIP' ]]; then
> 463 # for multi-homed AI servers use the "$serverIP"
> keyword
> 464 srv_address_ip="$serverIP"
> 465 else
> 466 srv_address_ip=`get_host_ip "$srv_address_hostname"`
> 467 fi
>
> Does below really get us the interfaces? Or a list of IP addresses? Isn't
> the interface ADDROBJ? Perhaps a different name for the variable.
>
> Also should tunnels be removed from the list? $GREP -v '->' If this is not
> pulled out then 'arithmetic syntax error' will be spewed from
> calculate_net_addrs.
>
> Why not use egrep for the exclusions instead? $EGREP -v
> '\\:|127.0.0.1|0.0.0.0'
> If you do this don't forget to include the tunnels issue as well.
>
> 493 interfaces=$(ipadm show-addr -p -o ADDR,STATE|\
> 494 $GREP -v '\\:'|$GREP -v '127.0.0.1' | \
> 495 $GREP -v '0.0.0.0' | $GREP ':ok$' | $SED
> 's/:ok$//')
>
>
> Nit - beggining should be beginning.
> 499 # check beggining address as that is what the DHCP
> server will
>
> Nit - otherwis should be otherwise
> 552 # strip the trailing space off match_nets
> as otherwis
>
> -----------------------------------------
> installadm.c
> -----------------------------------------
> I don't really care for the comments interspersed with the command
> string in the
> is_multihomed() function. To me it makes the function call harder to read.
> you also might need to strip out tunnels (->)?
>
> 491 (void) snprintf(cmd, sizeof (cmd),
> 492 /* use the shell to see if more than one interface has
> link */
> 493 "/usr/bin/[ `/usr/sbin/ipadm show-addr -p -o ADDR,STATE| "
> 494 /* strip out loopback, unconfigured DHCP, IPv6
> interfaces and */
> 495 /* those that are up -- have link, etc. */
> 496 "/usr/bin/egrep -v '\\:|127.0.0.1|0.0.0.0'|
> /usr/bin/grep ':ok$'|"
> 497 /* use wc(1) to count interfaces */
> 498 "/usr/bin/wc -l` -eq '1' ]");
>
> multiple calls to is_multihomed() seems a little expensive. Calling
> this once
> and setting a (possibly global) variable would be more efficient.
>
> 640 if (is_multihomed() != B_FALSE) {
> 662 if (is_multihomed() == B_TRUE) {
> 871 is_multihomed()?"\\$serverIP":server_ip,
> 882 is_multihomed()?"\\$serverIP":server_ip,
> dhcp_macro, dhcpbfile);
>
> -----------------------------------------
> installadm.h
> -----------------------------------------
> Shouldn't the Copyright include 2009 as well as 2010? Or the original
> creation
> of the file and 2010?
>
> -----------------------------------------
> server.xml
> -----------------------------------------
> Same copyright issue.
>
> For me the formating of the file was easier to read before.
> The original looks like:
>
> 43 <dependency
> 44 name='dns-multicast'
> 45 type='service'
> 46 grouping='optional_all'
> 47 restart_on='restart'>
> 48 <service_fmri value='svc:/network/dns/multicast:default' />
> 49 </dependency>
>
> The new one looks like:
>
> 35 <dependency name="dns-multicast" type="service"
> grouping="optional_all" restart_on="restart">
> 36 <service_fmri value="svc:/network/dns/multicast:default"/>
> 37 </dependency>
>
> The rest of the file is similarly condensed.
>
> -----------------------------------------
> setup-dhcp.sh
> -----------------------------------------
> I know this seems insignificant but why are you changing all the function
> definitions from 'name()' to 'function name'?
>
> Is there any significance to you using [[ ]] instead of [ ] for
> conditionals?
> The majority of the code only uses [ ] for conditionals.
>
> print_dhcp_macro_info seem more descriptive then print_service_macro_info.
> Ah, I see you separated out the message(s) into 2 functions. In that case
> lines 150-155 are duplicate output to lines 181-186. Would the customer
> ever see both sets of messages? If so then perhaps reworking the messages
> some more is in order. Don't want to nag the customer. :-)
>
> If you are running the Solaris DHCP server, use the following
> ....
>
> If you are running the Solaris DHCP server, use the following
> ....
>
>
> Nit - preferrably should be preferably
> 79 # overwrite is true, unless otherwise set (preferrably to
> append)
>
> Nit - escpaed should be escaped
> 496 # escpaed :'s (IPv6); next look for lines which
>
> Nit - settting should be setting
> 562 # an odd error indicates a failure in settting up network
> macros)
> _______________________________________________
> caiman-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
/* Build me as:
*
* cc -o test_multihomed test_multihomed.c -lsocket -lnsl
*
*/
#include <stdio.h>
/* includes needed to getifaddrs to work */
#include <sys/types.h>
#include <sys/socket.h>
#include <ifaddrs.h>
/* include for IF Flags */
#include <net/if.h>
/* include for inet_ntoa */
#include <arpa/inet.h>
void
debug_ifflags( uint64_t flags ) {
if ( flags & IFF_UP ) printf("IFF_UP ");
if ( flags & IFF_BROADCAST ) printf("IFF_BROADCAST ");
if ( flags & IFF_DEBUG ) printf("IFF_DEBUG ");
if ( flags & IFF_LOOPBACK ) printf("IFF_LOOPBACK ");
if ( flags & IFF_POINTOPOINT ) printf("IFF_POINTOPOINT ");
if ( flags & IFF_NOTRAILERS ) printf("IFF_NOTRAILERS ");
if ( flags & IFF_RUNNING ) printf("IFF_RUNNING ");
if ( flags & IFF_NOARP ) printf("IFF_NOARP ");
if ( flags & IFF_PROMISC ) printf("IFF_PROMISC ");
if ( flags & IFF_ALLMULTI ) printf("IFF_ALLMULTI ");
if ( flags & IFF_INTELLIGENT ) printf("IFF_INTELLIGENT ");
if ( flags & IFF_MULTICAST ) printf("IFF_MULTICAST ");
if ( flags & IFF_MULTI_BCAST ) printf("IFF_MULTI_BCAST ");
if ( flags & IFF_UNNUMBERED ) printf("IFF_UNNUMBERED ");
if ( flags & IFF_DHCPRUNNING ) printf("IFF_DHCPRUNNING ");
if ( flags & IFF_PRIVATE ) printf("IFF_PRIVATE ");
if ( flags & IFF_NOXMIT ) printf("IFF_NOXMIT ");
if ( flags & IFF_NOLOCAL ) printf("IFF_NOLOCAL ");
if ( flags & IFF_DEPRECATED ) printf("IFF_DEPRECATED ");
if ( flags & IFF_ADDRCONF ) printf("IFF_ADDRCONF ");
if ( flags & IFF_ROUTER ) printf("IFF_ROUTER ");
if ( flags & IFF_NONUD ) printf("IFF_NONUD ");
if ( flags & IFF_ANYCAST ) printf("IFF_ANYCAST ");
if ( flags & IFF_NORTEXCH ) printf("IFF_NORTEXCH ");
if ( flags & IFF_IPV4 ) printf("IFF_IPV4 ");
if ( flags & IFF_IPV6 ) printf("IFF_IPV6 ");
if ( flags & IFF_NOACCEPT ) printf("IFF_NOACCEPT ");
if ( flags & IFF_NOFAILOVER ) printf("IFF_NOFAILOVER ");
if ( flags & IFF_FAILED ) printf("IFF_FAILED ");
if ( flags & IFF_STANDBY ) printf("IFF_STANDBY ");
if ( flags & IFF_INACTIVE ) printf("IFF_INACTIVE ");
if ( flags & IFF_OFFLINE ) printf("IFF_OFFLINE ");
if ( flags & IFF_XRESOLV ) printf("IFF_XRESOLV ");
if ( flags & IFF_COS_ENABLED ) printf("IFF_COS_ENABLED ");
if ( flags & IFF_PREFERRED ) printf("IFF_PREFERRED ");
if ( flags & IFF_TEMPORARY ) printf("IFF_TEMPORARY ");
if ( flags & IFF_FIXEDMTU ) printf("IFF_FIXEDMTU ");
if ( flags & IFF_VIRTUAL ) printf("IFF_VIRTUAL ");
if ( flags & IFF_DUPLICATE ) printf("IFF_DUPLICATE ");
if ( flags & IFF_IPMP ) printf("IFF_IPMP ");
if ( flags & IFF_VRRP ) printf("IFF_VRRP ");
if ( flags & IFF_NOLINKLOCAL ) printf("IFF_NOLINKLOCAL ");
}
boolean_t
test_multihomed( void )
{
struct ifaddrs *ifa = NULL;
int num_ip4 = 0;
int num_ip6 = 0;
char addr_str[1024];
struct sockaddr_in *addr;
struct sockaddr_in6 *addr6;
if ( getifaddrs( &ifa ) == 0 && ifa != NULL ) {
/* Loop through addrs to see what there is */
do {
printf( "\n%s flags: ", ifa->ifa_name );
debug_ifflags( ifa->ifa_flags );
printf( "\n" );
/* Skip loopback, and Point to Point (tunnels) */
if ( ifa->ifa_flags & IFF_LOOPBACK
|| ifa->ifa_flags & IFF_POINTOPOINT ) {
printf("Skipping...\n");
continue;
}
/* Check for UP interfaces with proper address */
if ( ifa->ifa_flags & IFF_UP &&
ifa->ifa_flags & IFF_RUNNING ) {
void *addrp = NULL;
switch(ifa->ifa_addr->ss_family) {
case AF_INET:
addrp = (void*)(&((const struct sockaddr_in
*)ifa->ifa_addr)->sin_addr);
num_ip4++;
break;
case AF_INET6:
addrp = (void*)(&((const struct sockaddr_in6
*)ifa->ifa_addr)->sin6_addr);
num_ip6++;
break;
}
printf( "%s is UP, with addr %s\n", ifa->ifa_name,
inet_ntop(ifa->ifa_addr->ss_family, addrp,
addr_str, 1024));
}
else {
printf("Not Up and Running...\n");
}
}
while ( (ifa = ifa->ifa_next) != NULL );
freeifaddrs( ifa );
}
return( (num_ip4 > 1 || num_ip6 > 1 )?B_TRUE:B_FALSE );
}
int
main( int argc, char* argv[] )
{
printf("\nIs multi-homed = %s\n", test_multihomed()?"TRUE":"FALSE");
return(0);
}
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
