Thanks Jan. This looks good now!
Joe jan damborsky wrote: > Hi Joe, > > thank you very much for your comments. > Please see my response in line. > I have updated webrev with changes incorporated. > > Jan > > > On 04/21/09 16:32, Joseph J VLcek wrote: >> jan damborsky wrote: >>> Hi, >>> >>> could I please ask two people for reviewing changes for following >>> blocker ? >>> >>> 8262 'installadm create-service' shouldn't overwrite >>> /etc/netboot/wanboot.conf for Sparc, but notify user instead >>> >>> webrev: >>> http://cr.opensolaris.org/~dambi/bug-8262 >>> >>> Thank you very much, >>> Jan >>> >>> >>> modules affected: >>> ----------------- >>> * installadm tools (Sparc platform) >>> >>> testing done - please see attached test procedures >>> --------------------------------------------------- >>> >>> >>> ------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >> >> Hope this helps Jan. > > Definitely ! > >> >> usr/src/cmd/installadm/setup-service.sh >> --------------------------------------- >> >> 281 [ ! -f "${WANBOOT_CONF_SPEC}" ] && \ >> 282 /usr/bin/rm -f "${WANBOOT_CONF_SPEC}" >> >> Am I reading this wrong or is it not correct? >> >> If "NOT" file "${WANBOOT_CONF_SPEC}" then delete it? > > The intent was slightly different: > > If ${WANBOOT_CONF_SPEC} symbolic link is stale link > (target no longer exists), make sure the link is deleted. > > I would agree that it is not quite obvious, might it be > ok to put comment to explain or would you prefer to > implement it in different (more clear) way ? > > >> >> >> usr/src/cmd/installadm/setup-sparc.sh >> -------------------------------------- >> >> #1: >> --- >> >> 57 installconf="${image_path}/${SPARC_INSTALL_CONF}" >> >> Is this SPARC specific code? If so please describe that in the >> function comment. If not why prepend SPARC_ to INSTALL_CONF ? > > > Yes, all code in setup-sparc.sh is Sparc specific. > However, SPARC_INSTALL_CONF definition lives in installadm-common.sh > which contains both x86 as well as Sparc code, so this is the reason why > 'SPARC_' prefix was added. Please let me know if you prefer to remove it. > >> >> #2: >> --- >> >> 107 [ ! -d "$confdir" ] && /usr/bin/mkdir -p "$confdir" >> >> Consider setting the directory mode with the -m flag to mkdir. > > I have added -m 755. > >> >> #3: >> --- >> >> Question/Nit? >> >> Does it make sense to put the entire functionality in the block of >> code between lines 201 and 254 into it's own function or even >> incorporate it into function create_wanbootconf? > > I have created new function get_service_with_global_scope(). > >> >> #4: >> --- >> >> One too many "image_directory=" >> >> 230 image_directory=`/usr/bin/dirname >> "$root_file_location"` >> 231 image_directory=`/usr/bin/dirname >> "$image_directory"` > > The intent here is to strip last two levels from the path - > this is the reason why we see 'image_directory' twice. > Would you prefer to rename one of them to something else ? > >> >> >> #5: >> --- >> Wording suggestion and Sparc should be all caps SPARC >> >> 238 echo "Service $srv_dfl is currently used by Sparc" \ >> 239 "clients not explicitly associated with other" \ >> 240 "service by 'create-client' subcommand" >> >> To: >> >> echo "Service $srv_dfl is currently used by SPARC" \ >> "clients not explicitly associated with another" \ >> "service by the use of the 'create-client' subcommand" > > Changed. > >> >> #6: >> --- >> Wording suggestion and Sparc should be all caps SPARC >> >> 242 echo "Another service is currently used by Sparc" \ >> 243 "clients not explicitly associated with other" \ >> 244 "service by 'create-client' subcommand" >> >> To: >> echo "Another service is currently in use by SPARC \ >> "clients not explicitly associated with a" \ >> "client specific service." > > This message was removed when addressing Sarah's code review > comments. > >> >> #7: >> --- >> Wording suggestion and Sparc should be all caps SPARC >> >> 247 echo "Symbolic link has to be created in following way to" \ >> 248 "select $svc_name service for those Sparc clients" >> >> >> echo "To select service $svc_name for those SPARC clients" \ >> "use the following commands:" >> >> > > Changed. >
