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? 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 >> >
