Updated webrev at: http://cr.opensolaris.org/~jeanm/slim_5813_2/
Jean 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
