Sundar Yamunachari wrote:
> Sanjay Nadkarni wrote:
>> Sue Sohn wrote:
>>> 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.
>> While reviewing I noticed that the function can return success even
>> when the "service_data" is partially filled. Is this assumption
>> valid ? Do the values for each of the name value pairs need to be
>> sanity checked or is this already done when the file is written out.
>> Will this file be ever hand edited ? If so more checking is required.
> The service configuration information is saved only after all the
> values are are used to create other configuration files sych as
> menu.lst, DHCP macro, creating the AI image and registering the
> install service with mDNS. This file is not supposed to be hand edited
> but we don't prevent the edit.
> This service configuration will be converted in to SMF property group
> (bug 7218) and additional efforts to prevent hand-edits or validation
> may be throw-away code.
Thanks for the clarification. SMF property group is probably a better
way to go. In that case please ignore the comments.
-Sanjay
>
> - Sundar
>>
>> -Sanjay
>>
>>>
>>> > 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
>>> >
>>>
>>>
>>
>
