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.
>
> 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().
>
> 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.
Thanks,
Sundar
>
>
> Sue
>
