Updated versions are attached.
Comments are inline.
Jakub Hrozek wrote:
On 09/28/2009 05:03 AM, Dmitri Pal wrote:
> The patch comments give more details.
> The first two patches are two rounds of work related to "resolving"
> The event (when created) might have attributes that come from a
> Some of these attributes need to be resolved (replaced) at the execution
> Time and date are among them for example.
* 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
The reason for FLATDOT copying is that we want to have a final event
flat but we want to preserve the fact that original event was more
If the event was constructed and caller used a variable form collection
as addition to the event the event at this point (before copy) is a tree.
After copy it should be flat so normal would not work - it will leave
the tree structure.
If we use flat we will loose the fact that some part of the event was
Let me give you an example:
Say caller has a socket collection with:
The event then looks like:
The normal copy will leave the event hierarchical.
This would then require each data provider to try to express the tree
It is a burden.
So the design decision was that ELAPI flattens the event before passing
to the sinks.
So it can be flattened in two different ways:
I think the second one makes much more sense.
* some typos in comments, if you end up fixing something else in the
code, might be nice to fix them, too:
Fixed the typos.
The rest of the patch looks OK to me.
* typos in the commit message "vent -> event", "attribue",
* elapi_sprintf_item(): I don't understand the logic behind
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)
I explained the logic on IRC.
* 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
two ifs into one if you don't anticipate parsing anything else than
brackets [have "if (*runner == '%' && *(runner+1) == '(')]
while(*runner) instead of while(1) and checking for error condition to
return from the loop.
We agree that we will leave is as is for future enhancements.
* please use discard_const_p instead of memcpy on line 348. Or maybe
"const char *" for "start", the parameter "format" in
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).
are actually Ok since the code to call elapi_sprintf is commented out in
the first patch.
I just had to draw the line somewhere so I commented the code for it to
0003: Ack, but see below for explanation of the parentheses. The
you added in the unit test are legitimate, not sure if it warrants a
comment in the code :-)
I will leave the comment till next time I touch the file.
> 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
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.
sssd-devel mailing list
Engineering Manager IPA project,
Red Hat Inc.
Looking to carve out IT costs?