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.
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
>>
>
>
>
