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

Stephen Gallagher sgallagh at redhat.com
Tue Dec 22 19:53:24 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/22/2009 02:45 PM, Dmitri Pal wrote:

>> I'd prefer it to be a single variable, perhaps SSSD_TEST_VERBOSE=1
> 
> But think long term: it should probably be COMMON_TEST_VERBOSE then
> since eventually it will be broken out from the SSSD.
> And may be it should be one for COMMON and one for ELAPI since it would
> make sense IMO to break out eventually these two too.
> 

Fine with me.

> 
> I disagree and I think it is an unneeded overhead.
> git a -i is a good tool that I was not aware of but anyways with a huge
> patch like this it is just asking for trouble.
> If we think it should be reusable I can put it under common.
> This I can agree with then the makefile changes would make sense.
> 

I'd like to see it under common. I can see this having use beyond just
ELAPI.


>>>> Your unit test for copying and assignment is flawed. You need to print
>>>> both results and assert that they strcmp()==0. Also, it's dangerously
>>>> unsafe to do an assignment to the same variable to which you are
>>>> returning a value by reference. They need to be different.
>>>>
> 
> I do not understand what is wrong with line
>         ret = *((char **)ref_array_get(ra2, i, &ret));
> 
> Yes it will crash if the array element is NULL but how it is used is a
> perfectly valid use.
> It returns the pointer to the element in the array and we store char *
> in the array.
> We know that.  And it will never be NULL untill you are out of boundaries.
> It is also used from within the loop for up to the length and this
> guarantees that the returned value is not NULL and conversion can happen.
> With this unit test I illustrate the two different uses on the
> ref_array_get that can be implemented.
> One is the for loop and another is while loop. It is up to the collect
> to pick the right approach for his case.
>  
> 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
}


> 
> 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).


> 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.




- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAksxI7QACgkQeiVVYja6o6PSaACfcMcdnmjafn6GnzGruDJlLhEX
A4IAnjvpi2TI/rMO/dDRbln0682hQmqX
=44iE
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list