[SSSD] [PATCHES] ELAPI related patches

Jakub Hrozek jhrozek at redhat.com
Thu Oct 1 14:22:35 UTC 2009


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

On 09/28/2009 05:03 AM, Dmitri Pal wrote:
> Hello,
> 
> The patch comments give more details.
> The first two patches are two rounds of work related to "resolving" events.
> The event (when created) might have attributes that come from a template.
> Some of these attributes need to be resolved (replaced) at the execution
> time.
> Time and date are among them for example.
> 

0001:
* Is it possible to change the callbacks in elapi_resolve.c to static?
* Why FLATDOT in elapi_resolve_event()? I would expect FLAT or even NORMAL
* some typos in comments, if you end up fixing something else in the
code, might be nice to fix them, too:
 common/elapi/elapi_resolve.c:33 (resoltion)
 common/elapi/elapi_resolve.c:216 (prefere)
 common/elapi/elapi_resolve.c:216 (prepeare)

The rest of the patch looks OK to me.

0002:
* typos in the commit message "vent -> event", "attribue", "resoved"
* elapi_sprintf_item():  I don't understand the logic behind only
accounting the terminating '\0' when out_data has not been initialized
before -- I think you need to always account it and let
elapi_grow_data() realloc the buffer as needed, you never know what
length was before (it can be 1 byte or whatever)
* Is it necessary to add another '\0' after the switch? sprintf adds it
automatically, in default: it is added manually, only case: where this
helps is the COL_TYPE_STRING: one but I actually think it would be more
readable to have the there
* in elapi_parse_format() maybe it would be more readable to merge the
two ifs into one if you don't anticipate parsing anything else than
brackets [have "if (*runner == '%' && *(runner+1) == '(')] and also
while(*runner) instead of while(1) and checking for error condition to
return from the loop.
* please use discard_const_p instead of memcpy on line 348. Or maybe use
"const char *" for "start", the parameter "format" in elapi_parse_format
looks like it should be const char * anyway

By the way, the patches have reversed dependencies, which is not
critical as long as we apply them in one go, but worth noting - patch
0001 uses code from 0002 (elapi_sprintf_item, for example), usually it
should be the other way around (building upon previous patches).

0003: Ack, but see below for explanation of the parentheses. The ones
you added in the unit test are legitimate, not sure if it warrants a
comment in the code :-)

> The last patch fixes some warnings in the ELAPI example.
> I am running on RHEL5 and this is why I think I see these warnings.
> 
> In one of the previous mails it  was suggested to use less parentheses
> but it seems that compiler wants more :-)
> I wanted to draw attention to this fact since I do not plan to try to
> reduce the number of parentheses I usually use in conditional
> statements.

I guess what Martin meant is that if you used less parentheses, compiler
would issue a warning when you accidentaly used "=!" instead of "!="
because "a =! b" is correct C that says "assign negation of b to a".

Without parentheses, gcc warns about assignment used as truth operator,
which may have had draw attention to the fact that you did not want to
have any assignment whatsoever on that line.

	Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrEuyoACgkQHsardTLnvCVtLgCfXRoFu0GagDg6YhO3gdcoaO9b
AEQAn0vW4hR26GKRyV0xo7RfYJJd95KT
=rCc2
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list