Hi Evan,
my apologies - I accidentally hit 'Send' button,
I have couple of additional comments :-)
Thank you,
Jan
ai_trans.c
----------
96-108 might be simplified as:
...
if (handle->trans == NULL)
return (AI_TRANS_ERR);
/*
* Setup the transaction to modify the property
* group.
*/
if (scf_transaction_start(handle->trans, handle->pg) != 0) {
scf_transaction_destroy(handle->trans);
handle->trans = NULL;
return (AI_TRANS_ERR);
}
...
132 - nit: according to man page, scf_transaction_commit()
returns -1 on failure:
132 if (scf_transaction_commit(handle->trans) < 0)
->
132 if (scf_transaction_commit(handle->trans) == -1)
206 - 209 - 'value' and 'entry' are deallocated only if some kind
of failure happens. Where/when are those objects to be deallocated
if everything succeeds here ?
ai_utils.c
----------
52 - since ssize_t is not a pointer, I might recommend to
return 0 instead of NULL in case of failure.
61-62 - is this check valid ? Looking at the scf_limit man page,
it is not specified that 0 is invalid value.
66 - Why is MAXPATHLEN returned in case of failure ?
According to 52, Shouldn't be 0 returned in that case ?
79-80 - comment is not correct
84, 114, 154, 190, 252, 327, 395, 440, 557 -
it might be more appropriate if function returns
ai_errno_t instead of int
171 - It seems that return code doesn't reflect the
failure, since it was checked on 168 that property
group exists
268 - what happens, if scf_limit() fails in ai_get_scf_limit() ?
271, 464 - since scf_limit() doesn't include space for terminating
null byte, it perhaps should be:
271,466:
valuestr = malloc(vallen);
->
valuestr = malloc(vallen + 1);
342 goto out;
->
342 return (ret);
351 ret = AI_NO_MEM;
352 goto out;
->
351 return (AI_NO_MEM);
409,414:
goto out;
->
return (ret);
417 if ((ret = ai_end_transaction(handle)) != AI_SUCCESS) {
->
417 return (ai_end_transaction(handle));
464, 465, 499, 566, 595
- how is it handled, if scf_limit() fails in ai_get_scf_limit() ?
642 - nit:
if (handle != NULL) {
...
->
if (handle == NULL)
return;
...
634 - 635 - comment is not correct
643, 646, 656: nit
- please define and use 'unbind' as boolean_t
On 03/30/09 12:10, 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 ?
>
> 27,69,72 - since SUBDIRS is empty, I think this
> code could be removed and 59 simplified:
>
> 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
>
> 55 - according to comment at 51, it might be better if function
> returns ai_errno_t instead of int.
>
> 74 - ai_get_instance() might be used instead
>
>
> ai_trans.c
> ----------
>
> 60, 126, 156 - it might be better to return ai_errno_t instead of int
>
> 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
>>>
>>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
