Hi Sundar,
Sundar Yamunachari wrote: > Jan, > > I updated the webrev with the changes based on your review > comments. Please take a look at the webrev and let lne know. > > One of your comments suggested to combine lofiadm and mount together. > When I tried, it didn't work on OpenSolaros 05.08 version. So I am not > combining them. you are right, I have found that support for direct lofi mount was integrated in build 91 (bug 6384817). If we would like to allow to use installadm on older builds, we can't take advantage of this feature. I am sorry about the confusion. I have gone through the updated webrev and the changes look good to me. Thank you, Jan > > Thanks, > Sundar > > Sundar Yamunachari wrote: >> Jan, >> >> Thanks for the review. I will provide responses to my code. Sue >> already responded for her code. I am not able to get in to build >> machines to build and test. I will send the modified webrev once it >> is tested. >> >> >> jan damborsky wrote: >>> 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" >> I was going back and forth between OSInstall and install. I will >> change it to _OSInstall like the AI design doc say. >>> >>> >>> 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 ? >> This doesn't hold the actual macro. This holds the name of the macro >> and it is generated from boot_file. in theroy it could MAXNAMELEN+11 >> (dhcp_macro_<boot_file_name>, I could change it to MAXNAMELEN+12 >>> >>> 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); >> I will change it. >>> >>> 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. >> Initially I thought they were different ports. But this is not used >> by dns-sd. I will remove the sdport. >>> >>> 430 - Is "ai_webserver" name of TXT record correct ? >>> I think it should be "aiwebserver" instead. >> Yes. I have fixed in my copy while testing. >>> >>> 859 - just a nit: comment is not correct >> Yes. Will be fixed. >>> >>> 1008 - I think this is not necessary, since >>> fgets(3C) reads '\n' into 'buf' >> ok. I will remove the '\n' >>> >>> >>> 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 ? >> Good catch. multiboot used to be part of menu.lst but it is no longer >> user. We will use 'x86.microroot' instead. >>> >>> 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. >> We didn't catch it because the webserver is stripping the second':'. >> We will remove the second ':' in 83 and 86 >>> >>> >>> 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. >> I kept the $TMP_DHCP file for debug purposes. I will remove it. >>> >>> 68 macro+$1 >>> -> >>> 68 macro=$1 >> okay. >>> >>> 109,132,158: >>> net=`echo $n1.0` >>> -> >>> net="$n1.0" >> okay. >>> >>> 180 - just a nit (typo): >>> deson't -> doesn't >> okay. >>> >>> >>> cmd/installadm/setup-image.sh >>> ----------------------------- >>> 115 mkdir -p -- $1 >>> -> >>> 115 mkdir -p $1 >> mkdir is more tolerant than I thought :-) . I will fix it >>> >>> 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 >> Okay. >>> >>> 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. >> I don't want us to pick and choose files from the ISO. Doing it while >> creating image is the right thing. >>> >>> >>> cmd/installadm/setup-service.sh >>> ------------------------------- >>> 46,79: >>> # $2 - Service Type (_install._tcp) >>> -> >>> # $2 - Service Type (_OSInstall._tcp) >> okay. >>> >>> 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. >> True. In other place (lines 137-140), I am sleeping for 2 minutes. I >> will do the >>> >>> 114-115 - If dns-sd(1M) fails to register the >>> service, I think it should be killed. >> If the dns-sd registration failed, then it is not running. I can >> check whether it is running and kill it if needed. >>> >>> 146 - It seems this is fine for now, but >>> when we start using unicast DNS, check for >>> domain should be done as well. >> Right now, the service domain is always local. We need to have our >> own library rather than using dns-sd. So after November, we should >> work on that. >>> >>> >>> pkgdefs/SUNWinstalladm-tools/prototype_com >>> ------------------------------------------ >>> '/usr/sbin/installadm/webserver.py' >>> is missing >> This is Clay's code. I was using it to test my code by installing the >> package. This shouldn't be in the prototype_com with this webrev. I >> will remove it. >>> >>> >>> pkgdefs/SUNWinstalladm-tools/depend >>> ----------------------------------- >>> 51-52 - Is apache2 needed ? >> Yes. We need it to host install images. >>> >>> I think that SUNWtftp package should be added >> okay. >> >> Thanks, >> Sundar >>> >>> >>> >>> 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 >>> >> >> >> >
