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.
