Stephen Gallagher wrote:
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
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.
>> 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.
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.
>> 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.
>>
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.
> 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
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.
>> 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.
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.
>> 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.
See my comments above :-)
_______________________________________________
sssd-devel mailing list
sssd-devel(a)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/