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