Hi,
* I am not resending the COLLECTION patch. Ticket #350 has been open to
foster further discussion about the issue.
* 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
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/