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

Stephen Gallagher sgallagh at redhat.com
Tue Dec 22 16:10:52 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

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)

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)


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’

You did not add the necessary changes to common/elapi/Makefile.am and
configure.ac to build the new refarray directory.

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

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.

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.

Similarly, ref_array_create() needs to return an error if elemsz < 1.

In ref_array_destroy(), please change the code layout so that you first do
if (!ra) {
    TRACE_ERROR_STRING("Uninitialized array.", "Coding error???");
    return;
}

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.

Speling misteak at line 233 in ref_array.c. "retuning" should be "returning"

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

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.



Review of the elapi megapatch coming later.

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

iEYEARECAAYFAksw74wACgkQeiVVYja6o6MjVQCgqtX888+qQfV+hOSOoUIxxp1s
rQoAnicuy5Js/YSrNlvVZaH9CTlr3Bf3
=FykH
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list