Hi Sanjay,
Thank you very much for the review.
sanjay nadkarni (Laptop) wrote:
>
> After following the calls to save_service_data I am not convinced that
> it should be a void function.
>
> In save_service_data, there are number of places where failures can
> occur. These errors are printed out and the function returns. For
> example at line 824 save_service_data is called and the return is
> INSTALLADM_SUCCESS. This can result poor error handling further down
> the code path.
I have made quite a few changes to beef up the error handling.
save_service_data is no longer a void.
> installadm_util.c
> normalize_name_serivce:
> since you are returning a strduping, it might be simpler to strdup
> the passed in string and modify it, that way you avoid an unnecessary copy.
Yes, that is better, thanks. I also added a check for NULL return from strdup.
> read_service_data_file:
> would usage of strtok make the code simpler ?
Rewriting the function with strtok didn't make it look simpler to me. So, I
decided to leave it.
> write_service_data_file:
> need to check return values from fputs if it fails.
fixed.
> get_service_data:
> 372: normalize_service_name can return NULL but not checked.
added check.
> remove_data_service:
> 399: NULL return not checked.
added check and converted function to boolean_t, and then converted calling
function save_service_data to boolean_t, and checked return values in
installadm.c
> setup-service.sh
> 71: is it possible to simply the expression used there by using
> pgrep "dns-sd" or perhaps just pgrep "${service-name}"
>
> similar case for 369.
I played around a bit with pgrep, but didn't really see a way to simplify the
expression without risking false positives. I'm open to suggestions, otherwise
I'd rather leave it the way it is.
webrev is updated.
Sue
> -Sanjay
>
>
> Sue Sohn wrote:
>> Sundar Yamunachari wrote:
>>> Susan Sohn wrote:
>>>> Sundar,
>>>>
>>>> Thank you very much for the review. See below.
>>>>
>>>> On 03/06/09 09:25, Sundar Yamunachari wrote:
>>>>> Susan Sohn wrote:
>>>>>> Please review the changes for:
>>>>>>
>>>>>> 6128 installadm reuses /var/ai data and screw up manifest
>>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=6128
>>>>>>
>>>>>> and
>>>>>>
>>>>>> 7122 installadm stop kills /usr/bin/dns-sd for all services of
>>>>>> similar name
>>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7122
>>>>>>
>>>>>> which are posted at:
>>>>>>
>>>>>> http://cr.opensolaris.org/~sohn/6128_7122
>>>>>>
>>>>>> Thanks,
>>>>>> Sue
>>>>>>
>>>>>> _______________________________________________
>>>>>> caiman-discuss mailing list
>>>>>> caiman-discuss at opensolaris.org
>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>>> installadm.c:
>>>>>
>>>>> 468: Can you add a comment on why normalize_service() is needed for
>>>>> service_name?
>>>>
>>>> done
>>>>
>>>>> 708, 1019, 1119: Printing the service name along with txt record is
>>>>> better
>>>>
>>>> In all of those cases, the user has specified the service name on
>>>> the command line, so it seemed redundant to also print the service
>>>> name. I can put it in if you feel strongly.
>>> I feel that the port number is something the user doesn't know.
>>> txt_record is what we create internally. It doesn't make any sense
>>> for the user. If you add the service there, there will be some
>>> correlation.
>>>
>>> - Sundar
>>
>> Hi Sundar,
>>
>> I have made the changes and updated the webrev.
>>
>> Sue
>>
>>>>> 862-867: Should change the status only if we can successfully
>>>>> stopped the service. Move these lines after 878
>>>>
>>>> I originally had it there, but moved it up. My reasoning was that,
>>>> looking at setup-service, remove_service returns non-zero if the
>>>> service is not running. If the service is already not running, then
>>>> it is appropriate to set the status to off anyway. So moving those
>>>> lines up, to ensure the status was set to off, seemed better so that
>>>> the file stayed consistent with the actual status. Otherwise, it
>>>> would be possible to end up with the service not running but the
>>>> file status still set to on.
>>>>
>>>>> installadm_util.c:
>>>>> 39: get_a_free_tcp_port() declared in this file and installadm.c
>>>>> (line 54). Can you move this declaration to installadm.h?
>>>>
>>>> done
>>>>
>>>>> 43: installadm_system() declared in this file and installadm.c
>>>>> (line 55). Can you move this declaration to installadm.h?
>>>>
>>>> done
>>>>
>>>>> 141: Can You could ombine these lines to one return statement such as:
>>>>> return(read_service_data_file(path, data));
>>>>
>>>> After asking you privately, it turned out you meant line 380.
>>>> I will combine them.
>>>>
>>>> Sue
>>>>
>>>>> - Sundar
>>>>>
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
