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
