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
>
