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