Joe, thanks for review !
Jan On 04/21/09 23:40, Joseph J VLcek wrote: > 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. >> >
