Evan,
*usr/src/cmd/installadm/installadm-common.sh*:
35: SED is defined but unused while sed command is used in 247 and381.
*usr/src/cmd/installadm/installadm.c:
*174-191: Why the actions in if and else case are same? If these actions
(179-181) need to be taken for all cases, save the status from the line
174 and take these actions before returning based on status.
327-329: Can you move the message to installadm.h in the same format of
the other installadm messages?
337: The comment indicates that an error is returned where as the
function is a void function.
*usr/src/cmd/installadm/installadm_util.c:
*130: Change the comment from service data files to SMF properties group
209: pg_name is not checked for NULL. Why?
217: This comment still refers to the service data file.
*usr/src/cmd/installadm/test/installadm_test.c:
*The validity of argv[] is not checked in main and other functions use
argv[n] directly.
*usr/src/lib/libaiscf/Makefile:*
42: This line is commented out. If it is not needed, remove it.
- Just curious: The file ai_create.c contains only function
ai_delete_install_service(). So why it was not called ai_delete.c?
*usr/src/lib/libaiscf/ai_utils.c:*
79: Should be AI_SUCCESS (for success)
80: should be ai_errno_t (for failure)
126: If the property group already exists, can an error like "property
group exists already" be returned instead of AI_CONFIG_ERR
295: Check whether strdup is successful
372-375: In some of the error cases (352, 357 and 365), the
start_transaction was successful (341). Do you have to do a
end_transaction() here?
422: Same comment as the previous one.
468: It is better to check whether name and valuestr are NULL here
(rather than at 486) since you can return sooner with error.
504, 506, 519, 520: Check return value for strdup.
501-525: If there is failure after allocating space for some of the
properties, is it worth while to free up partially filled prop_list?
598, 612: Check return value for strdup
593-615: if there is failure after allocating space for some of the
property groups, is it worth while to free up partially filled pg_list?
634-635: The comment indicates that an error is returned where as the
function is a void function.
790: change to gettext("unknown Error")
Thanks,
Sundar
**Sundar Yamunachari wrote:
> Evan,
>
> I will look at installadm changes. If it goes faster then I will
> look at the rest of the code changes.
>
> - Sundar
>
> Evan Layton wrote:
>> My apologies for the delay. I have fixed the problems with the
>> mis-merge, sanity tested the changes and updated the webrev. I've
>> listed out the files and split things up between the library and
>> installadm. Just pick a subset of files and let me know what you're
>> looking at som we make sure everyting gets covered.
>>
>> And thanks again to Sue for catching the mis-merge!!!
>>
>> Thanks!
>> -evan
>>
>> installadm files:
>> usr/src/cmd/installadm/Makefile
>> usr/src/cmd/installadm/installadm-common.sh
>> usr/src/cmd/installadm/installadm.c
>> usr/src/cmd/installadm/installadm.h
>> usr/src/cmd/installadm/installadm_util.c
>> usr/src/cmd/installadm/server.xml
>> usr/src/cmd/installadm/setup-service.sh
>> usr/src/cmd/installadm/svc-install-server
>> usr/src/pkgdefs/SUNWinstall-libs/prototype_com
>>
>> libaiscf Library files:
>> usr/src/lib/Makefile
>> usr/src/lib/libaiscf/Makefile
>> usr/src/lib/libaiscf/ai_create.c
>> usr/src/lib/libaiscf/ai_trans.c
>> usr/src/lib/libaiscf/ai_utils.c
>> usr/src/lib/libaiscf/libaiscf.h
>>
>>
>> And if anybody is REALLY board here's the test files...
>>
>> optional files:
>> usr/src/cmd/installadm/test/installadm_test.c
>> usr/src/cmd/installadm/test/manual_tests
>>
>>
>> Evan Layton wrote:
>>> Sue pointed out that the webrev shows that I have a mis-merge so if
>>> you're looking at the changes please stop. I'll get the mis-merge
>>> fixed and send out a new webrev as soon as possible.
>>>
>>> Thanks,
>>> -evan
>>>
>>>
>>> Evan Layton wrote:
>>>>
>>>> We need to get the fix for 7218 code reviewed. There are around
>>>> 2000 lines of changes and new code so we'll need volunteers that
>>>> can take sections of the code to look at. I'm sending this out now
>>>> so the link is out there but I'll want to get things divided up
>>>> tomorrow morning and get folks looking at this tomorrow.
>>>>
>>>> The bug is 7218
>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7218
>>>>
>>>> Webrev:
>>>> http://cr.opensolaris.org/~evanl/7218/
>>>>
>>>> Thanks!
>>>> -evan
>>>> _______________________________________________
>>>> caiman-discuss mailing list
>>>> caiman-discuss at opensolaris.org
>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>
>>> _______________________________________________
>>> caiman-discuss mailing list
>>> caiman-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20090327/442824a9/attachment.html>
