[SSSD] [PATCHES] Collection, Ref Array, ELAPI

Dmitri Pal dpal at redhat.com
Wed Dec 23 20:59:09 UTC 2009


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 at 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/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-REFARRAY-New-referenced-array-object.patch
Type: text/x-patch
Size: 25258 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20091223/9ed61741/attachment.bin>


More information about the sssd-devel mailing list