On 04/08/09 13:31, Jean McCormack wrote: > Updated webrev at: > http://cr.opensolaris.org/~jeanm/slim_5813_2/ > > Jean
installadm.c 698 - srv_name is '\0' at this point, so you should use service_name instead. 777 - (nit) leftover extra blank line setup-tftp-links.sh 41 - update or remove comment installadm_common.sh 544 and 549 - remove leftover debug echo statements Sue > Jean McCormack wrote: >> >> Thanks for the review. Comments inline. >> >> Jean >> >> Susan Sohn wrote: >>> On 04/08/09 09:37, Jean McCormack wrote: >>>> I need 2 reviewers for >>>> >>>> 5813 installadm delete-service does not remove entry from vfstab >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5813 >>>> >>>> The webrev is at: >>>> http://cr.opensolaris.org/~jeanm/slim_5813/ >>>> >>>> Jean >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >>> >>> >>> General: >>> As we discussed on the phone, I'd rather see installadm.c calling a >>> function in setup-tftp-links to remove the vfstab entry rather than >>> calling setup-service, >>> since the vfstab entries are setup there. >> Done. >>> >>> installadm.c: >>> 797 - There will only be "old" smf information if the service existed >>> already before running create-service. So perhaps this code might be >>> better if it was moved to around 722, where the named_service exists. >> Agreed. Done. >>> >>> setup-service.sh: >>> 221 >>> ${GREP} ${IMAGE_BOOTDIR} >>> -> >>> ${GREP} "^${IMAGE_BOOTDIR}[ ]" >>> >>> with space and tab inside [] >> OK. It shouldn't really make a difference in this section of code >> since it was only a time optimization. The >> original code will get a few more hits but the end result won't change. >>> >>> 230 should use /tmp/vfstab.$$ rather than /tmp/vfstab >> Done. >>> 233-234 maybe use mv instead of cp and rm >> Done. >>> 277 - what if fields are tab separated? >> Changing the cut -d' ' -f1 to awk '{print $1}' to accommodate any >> white space. >> >> Jean >>> >>> Sue >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
