Hi Sundar, I have gone through all the files - those which are not referred below look good to me.
Thank you, Jan cmd/installadm/installadm.h --------------------------- line 55 - According to the AI design specification, I think it should be: 55 #define INSTALL_TYPE "_OSInstall._tcp" cmd/installadm/installadm.c --------------------------- 158 - casting to (char *) is redundant 161 dirname = fullpath -> 161 dirname = strdup(fullpath) 164-169 - This might be moved to the beginning of the function 174-175 - 0 is always returned, even if subcommand fails (for instance because of unknown option). If it is possible to use installadm in non-interactive mode as well, I think it might be better if non-zero is returned in case of failure, so that caller is aware of this. 242- is 128 bytes sufficient for DHCP macro ? 301-302, 313-314 might be simplified and then 235 could be removed (it also applies to 555-556, 614-615, 659-660): 301 cmdp = &cmds[0]; 302 (void) fprintf(stderr, "%s\n", cmdp->c_usage); -> 301 (void) fprintf(stderr, "%s\n", cmds[0].c_usage); 425-426, 621-622 - Is it necessary to define two different ports wsport and sdport ? I think that they should be the same, since they both provide the same kind of information - port on which AI service (AI webserver) can be reached. 430 - Is "ai_webserver" name of TXT record correct ? I think it should be "aiwebserver" instead. 859 - just a nit: comment is not correct 1008 - I think this is not necessary, since fgets(3C) reads '\n' into 'buf' cmd/installadm/create-client.sh ------------------------------- 241,269 - it seems these checks are redundant, they were already done on lines 160,192 particularly cmd/installadm/installadm-common.sh ----------------------------------- 69 - check for 'multiboot' is done, but on 71-92 no entry containing 'multiboot' is added - is the check on 69 correct ? 83,86 - ':' separator is added between web server address and net image path. As 'http://' prefix is used, I think ':' separator should be omitted, as it might be assumed that standard URL format is to be provided. cmd/installadm/setup-dhcp.sh ---------------------------- 57,87 - TMP_DHCP is populated here - since it seems it is only temporary variable, I think it should be removed when it is no longer needed. 68 macro+$1 -> 68 macro=$1 109,132,158: net=`echo $n1.0` -> net="$n1.0" 180 - just a nit (typo): deson't -> doesn't cmd/installadm/setup-image.sh ----------------------------- 115 mkdir -p -- $1 -> 115 mkdir -p $1 143 & 150 can be combined in following way: 143 lofi_dev=`lofiadm -a $file` > /dev/null 2>&1 150 mount -F hsfs -o ro $lofi_dev $MOUNT_DIR > /dev/null 2>&1 -> mount -F hsfs $file $MOUNT_DIR > /dev/null 2>&1 Then 167 & 168 can be combined as well: 167 umount $MOUNT_DIR > /dev/null 2>&1 168 lofiadm -d $lofi_dev > /dev/null 2>&1 -> 167 umount $MOUNT_DIR > /dev/null 2>&1 205-220 - Just a thought - at this point it seems we will be copying bunch of stuff we actually don't need - but I feel that the right thing to do might be to take care of this in Distro Constructor and delete all unnecessary files there - some finalizer script could probably do the job. cmd/installadm/setup-service.sh ------------------------------- 46,79: # $2 - Service Type (_install._tcp) -> # $2 - Service Type (_OSInstall._tcp) 57-59 - Is it assured that enough time is given for the look up process ? It seems it might fail on fast machines just because 'grep' is called to early before dns-sd is given a chance to finish its job. 114-115 - If dns-sd(1M) fails to register the service, I think it should be killed. 146 - It seems this is fine for now, but when we start using unicast DNS, check for domain should be done as well. pkgdefs/SUNWinstalladm-tools/prototype_com ------------------------------------------ '/usr/sbin/installadm/webserver.py' is missing pkgdefs/SUNWinstalladm-tools/depend ----------------------------------- 51-52 - Is apache2 needed ? I think that SUNWtftp package should be added Sundar Yamunachari wrote: > Hi all, > > Please review the code for installadm > > 3640 Installadm tools to manage services > http://defect.opensolaris.org/bz/show_bug.cgi?id=3640 > > > 3641 installadm tools to manage clients > http://defect.opensolaris.org/bz/show_bug.cgi?id=3641 > > Webrev: > http://cr.opensolaris.org/~ysundar/installadm > > The code covers the following functionality: > > 1. create-service: > - Setup the netimage directory (from iso or another netimage) > - Create a DNS service > - Start a web server associated with the DNS service > - Create DHCP server if needed > - Create DHCP network and add addresses to the DHCP table (if needed) > - Create the DHCP macro for this netimage (and service) > - Create bootfile in the /tftpboot directory > - All the services (DHCP, TFTP, and mDNS) are setup and enabled if > they are not running > > 2. delete-service > - Remove the service > - Remove the image if needed > > 3. start-service > - Create a service with given name > > 4. stop-service > - remove the given service > > 5. create-client > - Create bootfile in the /tftpboot directory with the name derived > from MACaddress > - Create a DHCP macro for this client > - Enable tftp service if not running > > 6. delete-client > - Remove the tftp bootfile > > Additional features: > > 1. Apache webserver will be configured at a port 5555 to host images > (zlibs). When the image is setup, a link will be created under Apache > docroot to point to the net image directory. The client will access the > zlib through the link. An apache configured file will be shipped with > this package to start a webserver at port 5555. > > 2. For each service, an AI webserver will be started. The code for the > AI webserver will be reviewed separately but the webserver > (webserver.py) is used in this code. > > Code organization: > > - The main code is in installadm.c. It parses the user input and checks > the options. Based on the options, call one more scripts. Each service > or task is in a separate script. The scripts are: > > create-client.sh; Create a client > delete-client.sh; Remove a client > setup-image.sh: Add/Remove netimages and creates link to webserver > setup-dhcp.sh: DHCP table creation, Add network, add ip addresses, > create DHCP macro etc. > setup-service.sh: Register/lookup/remove service, start/stop aI web > server. Uses DNS-SD > setup-tftp-links.sh: creates bootfile under /tftpboot > installadm-common: Common functions between create-client.sh and > setup-tftp-links.sh > > Thank you, > Sundar & Sue > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
