Sundar, Looking good. Just one last thing below.
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. 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 >>>> >>> >> >
