jan damborsky wrote:
> 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);
> }
> ...
OK fixed.
>
>
> 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)
>
changed.
>
> 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 ?
>
I'm not sure I follow here. We don't return from this function without running
past this code so the calls to scf_value_destroy() and scf_entry_destroy() will
always get called unless 'value' and 'entry' are NULL.
>
> 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.
Yes this is a valid check. If scf_limit fails it -1 which is waht we're
checking
for. However the comment is incorrect and should state that if scf_limit fails
we return MAXPATHLEN.
* Return:
* ssize_t - Success
* MAXPATHLEN - on failure of scf_limit
>
> 66 - Why is MAXPATHLEN returned in case of failure ?
> According to 52, Shouldn't be 0 returned in that case ?
comment fixed...
>
> 79-80 - comment is not correct
>
fixed
> 84, 114, 154, 190, 252, 327, 395, 440, 557 -
> it might be more appropriate if function returns
> ai_errno_t instead of int
fixed
>
> 171 - It seems that return code doesn't reflect the
> failure, since it was checked on 168 that property
> group exists
you're right the return code is incorrect. I added "AI_PG_DELETE_ERR,
/* Failed to delete PG */" in the header file and this now returns that
error.
>
> 268 - what happens, if scf_limit() fails in ai_get_scf_limit() ?
>
vallen get MAXPATHLEN in it.
> 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);
changed.
>
>
> 342 goto out;
> ->
> 342 return (ret);
changed
>
>
>
> 351 ret = AI_NO_MEM;
> 352 goto out;
> ->
> 351 return (AI_NO_MEM);
changed.
>
>
> 409,414:
> goto out;
> ->
> return (ret);
changed.
>
>
> 417 if ((ret = ai_end_transaction(handle)) != AI_SUCCESS) {
> ->
> 417 return (ai_end_transaction(handle));
I added another function that needs to be called here in case the call to
end_transaction fails. This fucntion (ai_abort_tranaction) will clean things up
if there is a transaction failure. For more on this see my response to Sundar's
comments.
>
>
> 464, 465, 499, 566, 595
> - how is it handled, if scf_limit() fails in ai_get_scf_limit() ?
I don't understand what you mean here. In each instance we'll have MAXPATHLEN
as
the value in vallen and we'll use that for getting the value.
>
>
> 642 - nit:
> if (handle != NULL) {
> ...
> ->
> if (handle == NULL)
> return;
> ...
done.
>
>
> 634 - 635 - comment is not correct
>
fixed
>
> 643, 646, 656: nit
> - please define and use 'unbind' as boolean_t
Done
Thanks for the comments!!!
-evan
>
>
>
>
>
> 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
>
