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.
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.
read_service_data_file:
would usage of strtok make the code simpler ?
write_service_data_file:
need to check return values from fputs if it fails.
get_service_data:
372: normalize_service_name can return NULL but not checked.
remove_data_service:
399: NULL return not checked.
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.
-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
