Hi Jan, Thank you very much for your code review comments. See my responses below to selected comments. The rest of the comments will be addressed by Sundar.
> 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 ok > 161 dirname = fullpath > -> > 161 dirname = strdup(fullpath) ok > 164-169 - This might be moved to the beginning > of the function yes, that is better. > 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. I agree. We will fix that. > 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 At 160, we are just verifying that there *is* an IMAGE_PATH value. At 192, I am not seeing anything about IMAGE_PATH? For 241 and 269, if you mean 227 and 240, the first verifies that the path actually exists, the second verifies that it is a valid boot image. Is that what you're looking at and does that answer your question? Thanks again for the review, Sue > 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 > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
