Susan Sohn wrote: > Sundar, > > Looking good. Just one last thing below. Thanks Sue. I will take care of the last thing. > > Sundar Yamunachari wrote: >> Sue, >> >> Thanks again for review. Accepted all the suggestions and the >> updated webrev is at the same place >> http://cr.opensolaris.org/~ysundar/3976 >> >> - Sundar >> >> Susan Sohn wrote: >>> Sundar, >>> >>> A few more comments below. >>> >>> Sue >>> >>> >>> Sundar Yamunachari wrote: >>>> Sue, >>>> >>>> Thanks for the review. See my response below. I have updated the >>>> webrev at http://cr.opensolaris.org/~ysundar/3976/ with the changes >>>> based on the comments I accepted. >>>> >>>> Thanks, >>>> Sundar >>>> >>>> Susan Sohn wrote: >>>>> >>>>> Sundar Yamunachari wrote: >>>>> > Hi Jan and Sue: >>>>> > >>>>> > Please review the code for bug fixes. >>>>> > >>>>> > The webrev is at: http://cr.opensolaris.org/~ysundar/3976/ >>>>> > >>>>> > 3976 AI DHCP lease is short and not being extended >>>>> > http://defect.opensolaris.org/bz/show_bug.cgi?id=3976 >>>>> > >>>>> > 4070 Client booted with AI image cannot access outside the >>>>> subnet >>>>> > http://defect.opensolaris.org/bz/show_bug.cgi?id=4070 >>>>> > >>>>> > 3968 installadm does not create directory >>>>> > http://defect.opensolaris.org/bz/show_bug.cgi?id=3968 >>>>> > >>>>> > 3945 installadm delete-service doesn't remove the target >>>>> directories >>>>> > http://defect.opensolaris.org/bz/show_bug.cgi?id=3945 >>>>> > >>>>> > Thanks, >>>>> > Sundar >>>>> >>>>> >>>>> >>>>> Hi Sundar, >>>>> >>>>> installadm.c >>>>> ------------ >>>>> General: bug 3945 says the target directory isn't being deleted, >>>>> but the code changes at 566 look like it's the service that wasn't >>>>> being deleted? >>>> The problem happened because, there were issues with >>>> remove_service_data() code. When I asked to remove one entry, it >>>> was corrupting everything because of the way I used strtok (see the >>>> change made in 1289). >>>> The change in 566 is to make sure that the service record is >>>> deleted when delete-service is called >>>>> >>>>> 1259 - not in your changes, but should there be an error message >>>>> here? >>>> If the file doesn't exists that means we don't need to remove the >>>> entry. >>> >>> So there are valid cases that remove_service_data would be called >>> and that file wouldn't exist? Maybe add a comment? >>> >>>>> 1269 - if you intended for this file to be in /var/tmp rather than >>>>> /var, you're missing the final "/" in "/var/tmp/". And maybe call >>>>> it /var/tmp/ai.<pid> or /var/tmp/installadm.<pid> ? >>>> okay. I will change it to /var/tmp/installadm.<pid> >>>>> >>>>> 1281 - before you return, you need to cleanup tmp_fp and >>>>> associated file >>>> okay. Actually tmp_file is a local variable and so it should not be >>>> freed in 1324. I will use fclose(). >>> >>> 1281 - Don't you want fclose(tmp_fp) rather than fclose(tmp_file)? > > You should also remove the empty file that you created in /var/tmp. I remove the file if there is a failure. I have updated the webrev in case you want to look.
Thanks, Sundar > > Other than that, I am fine with the code and don't need to see another > webrev. > > Thanks, > Sue > > >>> >>>>> 1299 - Once you get service_name and directory, why not do the >>>>> check on 1312? Why do you look for file and data? In other words, >>>>> it seems like 1299-1306 could be deleted? >>>> Right now I am using only service_name and directory because I am >>>> deleting only directory now. We will be deleting boot file and >>>> directory corresponding to the web server after November. So I >>>> don't want to delete the code now. >>>>> >>>>> 1309 This comment needs to be rewritten to reflect what code is >>>>> actually doing (i.e., only writing line if it isn't a match to >>>>> existng service/image) >>>> okay. >>>>> >>>>> setup-dhcp.sh >>>>> ------------- >>>>> 165 - should the error message be expanded here so that if you >>>>> can't get the default route, you tell the user that they won't be >>>>> able to access outside the subnet or whatever the implications >>>>> would be? And/or say "continuing anyway", since you don't abort? >>>> okay. It is kind of difficult to tell them that the client is the >>>> one that is affected. I will change it to "Cannot get default >>>> router. Please check whether default route is configured". We don't >>>> abort for any DHCP errors. I don't want to add "continuing anyway" >>>> just for this message. >>> >>> ok, but you have a typo: fdefault->default >>> >>> Sue >>> >>>> Thanks, >>>> Sundar >>>>> >>>>> >>>>> Sue >>>>> >>>> >>> >> >
