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