Stephen Gallagher wrote:
On 12/14/2009 04:59 PM, Dmitri Pal wrote:
> Hi,
> This is resubmission of the two earlier withdrawn patches (Ref Array and
> ELAPI) and addition of a new patch to collection.
> For details see the patch comments.
> Note about ELAPI patch: it is huge so it is zipped. This patch includes
> the first cut of eliminating the elapi_test directory altogether. The
> directory is not built any more so there is no need to spend much time
> looking at the files that were updated in this directory. They will be
> either removed in follow up patches or moved to elapi directory.
Patch 0001:
Nack.
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.
Do not leave a semicolon at the end of the do...while(0) loop for
COLOUT. The whole purpose of using do...while(0) is so that it is safe
to apply a semicolon when the macro is used later in the code. (Some
compilers throw an error if preprocessed code has two adjacent semicolons)
Ok. Will do.
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.
Patch 0002:
Nack.
../../../../common/elapi/refarray/ref_array.c: In function
ref_array_debug:
../../../../common/elapi/refarray/ref_array.c:260: warning: format %d
expects type int, but argument 2 has type size_t
I will take a look at this one.
Will do.
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.
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?
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.
The grow_by parameter needs to have a default setting if 0 is
passed.
Right now, it's possible to pass 0 and the realloc will create a buffer
of the same size as the previous buffer, potentially resulting in a
buffer overrun vulnerability when we copy in the new value.
Ah, good catch, agree.
Will do.
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.
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.
In ref_array_destroy(), please change the code layout so that you
first do
if (!ra) {
TRACE_ERROR_STRING("Uninitialized array.", "Coding error???");
return;
}
Makes sense.
Will do.
if (ra->refcount < 1) {
/* Should never be here...
* This can happen if the caller by mistake would try to
* destroy the object from within the callback. Brrr....
*/
TRACE_ERROR_STRING("Reference count is 0.", "Coding error???");
}
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.
Speling misteak at line 233 in ref_array.c. "retuning"
should be
"returning"
Ok. Will do.
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.
Instead of having your debug function call printf directly (which is
buffered and might notprint 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 :-)
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.
_______________________________________________
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/