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

Jeff Schroeder jeffschroed at gmail.com
Tue Dec 22 17:48:13 UTC 2009


On Tue, Dec 22, 2009 at 9:44 AM, Dmitri Pal <dpal at redhat.com> wrote:
> 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  > expects type  , 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 at 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/ _______________________________________________ sssd-devel mailing list sssd-devel at lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel

Dmitri, your mua completely ate that message. Either it stripped all
newline characters or gmail freaked out. Just wanted to give you a
heads up.

-- 
Jeff Schroeder

Don't drink and derive, alcohol and analysis don't mix.
http://www.digitalprognosis.com



More information about the sssd-devel mailing list