On 04/08/09 14:42, Jean McCormack wrote: > Susan Sohn wrote: >> 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. > You're right. Caught it in one place but not the other. >> 777 - (nit) leftover extra blank line > Done >> >> setup-tftp-links.sh >> 41 - update or remove comment >> > Removed >> installadm_common.sh >> 544 and 549 - remove leftover debug echo statements >> > Done. > > Do you need another code review?
No need, thanks. Sue > Jean >> 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 >>> >> >
