Jan,
Thanks for your review.
- Sundar
jan damborsky wrote:
> 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
>>>>
>>>
>>>
>>>
>>
>
