jan damborsky wrote:
> Hi Evan,
>
> please see below for libaiscf comments.
>
> Thank you,
> Jan
>
>
> libaiscf/Makefile
> -----------------
> general comment - I can see that mix of ${} and $()
> is used for variables - I might recommend to pick
> one of them - perhaps $(), since it seems to be used
> more and also in other Makefiles.
>
> 42 - might this line be removed ?
removed.
>
> 27,69,72 - since SUBDIRS is empty, I think this
> code could be removed and 59 simplified:
It could be but it doesn't hurt to have it here and we may eventually want to
add some subdirs. For example we may want to add one for the test directory at
some point. I'd rather leave this if that's ok.
>
> 59 all: $(HDRS) static dynamic .WAIT $(SUBDIRS)
> ->
> 59 all: $(HDRS) static dynamic
>
> ai_create.c
> -----------
>
> since there is only ai_delete_install_service() function
> defined there, it might be better to use ai_delete.c
> name of the file instead of ai_create.c
This was originally named for the create function that was in here. However we
found it wasn't needed and the function was removed. I wasn't going to change
the name of the file but since both you and Sundar mentioned this I've renamed
it to ai_delete.c
>
> 55 - according to comment at 51, it might be better if function
> returns ai_errno_t instead of int.
ai_errno_t is an int ;-)
I change it to ai_errno_t;
>
> 74 - ai_get_instance() might be used instead
changed.
>
>
> ai_trans.c
> ----------
>
> 60, 126, 156 - it might be better to return ai_errno_t instead of int
changed.
Thanks! on to the next set of comments :-)
-evan
>
> On 03/27/09 16:17, Evan Layton wrote:
>> Thanks Jan!
>>
>> We'd like to get this done by close of business on Tuesday if possible
>> so we have some time to make the changes from the comments we receive.
>>
>> -evan
>>
>> jan damborsky wrote:
>>> Hi Evan,
>>>
>>> I could take a look at libaiscf library.
>>> When you would like to have it done ?
>>>
>>> Jan
>>>
>>>
>>> On 03/26/09 22:41, 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
>>>
>>
>
