Dmitri Pal wrote:
Hi,
* I am not resending the COLLECTION patch. Ticket #350 has been open to
foster further discussion about the issue.
I mean that it can be acked and pushed as is and we can look at the
issue separately.
* The refarray is now made common and the attached (or should I say
attacked? :-) ) patch addresses ALL issues that have been brought up.
The ELAPI patch will be sent later. I need to update it to factor in the
change in the location of the ref-array.
Also I checked that the patch will get much smaller if I ask it to track
deletes or moves/copies.
It actually gets twice smaller.
It is really not that big (read bad :-) ). It has about 2000-3000 lines
of new code thanks for async processing (not that I wanted it to happen
:-) )...
But all the rest is just due to the cleanup. For example the module
elapi_dispatcher had too many functions that were not related to
dispatcher so they got moved around to other modules.
Same with elapi_sink, elapi_log etc.
Now the code is much better arranged in layers and objects.
Also the wiki is your helper in review. It has now the write up about
ELAPI that I created before I submitted this patch last time.
Happy holidays!
Talk to you next year!
Dmitri
Stephen Gallagher wrote:
> On 12/22/2009 03:03 PM, Dmitri Pal wrote:
>
>>>> I do not see why anything should be changed here.
>>>>
>>> The problem is the use of "ret" in both the return from the
>>>
> function and
>
>>> as the memory location for the copied code. It's the same variable
>>> having data returned to it in two different ways from the same function
>>> call. This is dangerous. It should look like
>>>
>>> ret1 = *((char **)ref_array_get(ra2, i, &ret2));
>>> if (strcmp(ret1, ret2) != 0) {
>>> //FAIL
>>> }
>>>
>>>
>>>
>> Ah now I understand what you are talking about.
>> Yes I agree it is generally dangerous and should be discouraged but the
>> unit-test checks that even if it is done this way it is Ok.
>> I should probably just add a comment about it to discourage this kind of
>> use and explain that this is a test to validate that it will not crash
>> if it is done this way.
>>
> Fine, but please implement additionally the test I described above, as
> it will prove that the two return values are equivalent.
>
>
>>>> When you have time to do this.
>>>> I have a bigger fish to fry.
>>>> I am not arguing that these changes would make sense.
>>>> I argue that these changes are not required for the first patch that
>>>> introduces the interface.
>>>> If can't be expected to be perfect from get go.
>>>> It should be good enough to be able to move on with main goals.
>>>> Just a difference in the approach.
>>>>
>>> The whole purpose of a review is so that only stable code reaches the
>>> repository (hopefully).
>>>
>>>
>> IMO this is a flaw in the process.
>>
> Stable code is not a flaw :)
>
>
>>>> Hm? I do not buy it. If you want to test the validity of the
>>>>
> object you
>
>>>> check if it is NULL rather then call a length function.
>>>> The whole point of such simplified inteface is that you check for the
>>>> object validity once at the top and then just get it length when you
>>>> need to.
>>>> something like
>>>> int myfun(ref_array *ra)
>>>> {
>>>> ...
>>>> if (!ra) {
>>>> ...
>>>> return EINVAL;
>>>> }
>>>> len = ref_array_len();
>>>> ....
>>>> }
>>>> Now why check for ra twice and add overhead of error handling to
>>>>
> a very
>
>>>> simple function that returns a property of the object?
>>>> It is like a get method in C++. It just returns value.
>>>>
>>> I understand what you're saying, but you're attempting to enforce a
>>> coding standard in a function prototype. You can't know how a consumer
>>> of the API is going to use this call, so you have to protect
>>>
> against it.
>
>>> A NULL check is a very cheap operation.
>>>
>>>
>>>
>> This is exactly what I do internally:
>>
>> /* Get length */
>> uint32_t ref_array_len(struct ref_array *ra)
>> {
>> TRACE_FLOW_STRING("ref_array_len", "Entry");
>>
>> if (!ra) {
>> TRACE_ERROR_STRING("Uninitialized argument.", "");
>> return 0;
>> }
>>
>> TRACE_FLOW_STRING("ref_array_len", "Exit");
>> return ra->len;
>> }
>>
>
>> The function is written this way for convenience so that I do not need
>> to check and handle the error (and this is where the cost is, not the
>> check itself).
>> This is the whole point and value of the call.
>> You just asking to make it less usable and force the caller to add more
>> code in the error handling for now reason.
>> IMO it is a wrong approach.
>>
> How about setting errno=EINVAL in the if(!ra) block? If the caller
> cares, they can check its value if they get zero back.
>
> I'm not a fan of errno, but this might fit your workflow better as you
> describe it.
>
>
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
------------------------------------------------------------------------
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/