[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