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.
-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
> >
>
>
