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

Stephen Gallagher sgallagh at redhat.com
Tue Dec 22 18:01:20 UTC 2009


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

On 12/22/2009 12:44 PM, Dmitri Pal wrote:
> Stephen Gallagher wrote:
>>
>> COL_DSP_INDEX should not allow you to pass an index greater than one
>> past the current size of the collection. This should throw an error
>> rather than just placing it at the end, because it indicates that the
>> calling program has a flawed expectation of the size of the collection.
>>
>> In general, since the implementation is a linked-list, you almost
>> certainly don't want to allow retrieval access by index, since every
>> time you insert something, you change the indexes of subsequent
>> requests. The expectation of a program inserting a value by index is
>> usually that they will be able to retrieve that entry by requesting the
>> same index later. It's much safer to force them to search or walk the
>> collection to find the entry they are looking for.
> 
> This logic have been there for 6 months.
> I will open a ticket for that. It should not block a review for this patch.
> 

Agreed.

> 
> 
>>
>> It would be preferable if the verbosity could be set by an environment
>> variable (in addition to the commandline flag) (the unit tests in the
>> server directory use "CK_VERBOSITY=verbose", so it wouldn't be a
>> terrible idea to use the same variable)
>>
> 
> Should it be one variable per component or should it be same variable
> for collection, INI, ELAPI and other tools?
> Will do.
> 

I'd prefer it to be a single variable, perhaps SSSD_TEST_VERBOSE=1


>> You did not add the necessary changes to common/elapi/Makefile.am and
>> configure.ac to build the new refarray directory.
> 
> I think these changes are in the second patch. They have not been
> included intentionally because it was hard to split patches for the
> review purposes.
> 

Patches need to be reviewable independently so that dependency patches
like this one can theoretically be committed while subsequent patches
are still under review.

Please fix this.

>>
>> As above, I'd like to see the unit test verbosity set by an environment
>> variable. You also left the print statements in main() outside of RAOUT()
>>
> 
> Will do.
> Same or different?

Same as above.

> 
>> 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.
>>
> 
> Can you point to the line number here?
> Probably will do.

Line 149 of ref_array_ut.c

>>
>> Similarly, ref_array_create() needs to return an error if elemsz < 1.
>>
> 
> Can size_t be negative? I thought it is always positive. So it should
> check for 0.
> 

You're right, size_t is unsigned. So checking for zero is fine. I
couldn't remember if it was signed or unsigned.

> Comment: These are all correct suggestions except that ref_array so far
> is sort of "quick and dirty" internal interface.
> The whole point is not add these "niceties" until it becomes clear if it
> should be an externally usable interface or not.
> This is why I put it under ELAPI because it is not clear how reusable it is.
> I am leaning more to opening a bug and saying to address these issues if
> ref_array becomes a public interface.
> So far I agree that it is a good defensive programming approach but an
> overhead for a specific situation.
> 
> Will do.

Internal interfaces should be as correct as possible still


>> And then have the rest of the code two fewer indentation levels. It's
>> easier to read. Please also note that you need to check that the
>> refcount is < 1, not just that it's zero. A negative value could
>> indicate a memory error or a forgotten initializer somewhere.
>>
> 
> It is unsigned. It can't be less than 0.

Sorry, I misread. That's fine.

>> ref_array_len() needs to have a way to return an error instead of
>> returning 0 when ra isn't valid.
>>
>> I'd recommend that you want:
>> int ref_array_len(struct ref_array *ra, uint32_t *ret)
>> {
>>     TRACE_FLOW_STRING("ref_array_len", "Entry");
>>
>>     if (!ra) {
>>         TRACE_ERROR_STRING("Uninitialized argument.", "");
>>         return EINVAL;
>>     }
>>
>>     TRACE_FLOW_STRING("ref_array_len", "Exit");
>>     *ret = ra->len;
>>     return EOK;
>> }
>>
> 
> The whole point of the function was not check for an error and suppress it.
> I would rather add an error as a parameter if you insist.
> Again good defensive practice but IMO an overhead here.

Not just good defensive practice. A consumer of this API may use the
result of this call as the source of a conditional. If you return 0 for
an invalid structure, then they may still attempt to use the object.


>> Instead of having your debug function call printf directly (which is
>> buffered and might not print completely), I'd recommend that this
>> function should allocate memory to store the complete output string and
>> return a pointer to it. Then the caller of ref_array_debug() can choose
>> to output or store it to a file in whatever manner they choose.
>>
> 
> Too much work.
> It is sufficient for my debugging purposes.
> Patches are welcome :-)
>  

Agreed. We can improve on this later.

>>
>>
>> Review of the elapi megapatch coming later.
>>
> 
> If you agree with my assessment I will provide fixes for items that I
> said I "will do".
> I will open a ticket for collection and will defer the rest.
>

Please see my comments above.


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

iEYEARECAAYFAksxCXAACgkQeiVVYja6o6OrbwCcCz7EtaKZYUh56XnVBljxWJYn
f0cAmwaC5LTG3M6CcmbX63z7gNVRyP8V
=tm2e
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list