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.
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.
-----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
Updated versions are attached. Comments are inline.
Jakub Hrozek wrote:
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?
Done
- Why FLATDOT in elapi_resolve_event()? I would expect FLAT or even NORMAL
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 hierarchical. 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 hierarchical.
Let me give you an example: Say caller has a socket collection with: port timeout peer The event then looks like: date time host socket port timeout peer message
The normal copy will leave the event hierarchical. This would then require each data provider to try to express the tree hierarchy. 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: FLAT case: date time host port timeout peer message
FLATDOT case: date time host socket.port socket.timeout socket.peer message
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: common/elapi/elapi_resolve.c:33 (resoltion) common/elapi/elapi_resolve.c:216 (prefere) common/elapi/elapi_resolve.c:216 (prepeare)
Fixed the typos.
The rest of the patch looks OK to me.
0002:
- typos in the commit message "vent -> event", "attribue", "resoved"
Fixed.
- 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)
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
Addressed
- 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.
We agree that we will leave is as is for future enhancements. https://fedorahosted.org/sssd/ticket/215
- 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
Fixed.
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 compile cleanly.
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 :-)
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 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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
0001:
- Is it possible to change the callbacks in elapi_resolve.c to static?
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct in the first patch, rather than non-static in first patch and fixed to static in second.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/05/2009 07:55 AM, Jakub Hrozek wrote:
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
0001:
- Is it possible to change the callbacks in elapi_resolve.c to static?
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct in the first patch, rather than non-static in first patch and fixed to static in second.
Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
Jakub Hrozek wrote:
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
0001:
- Is it possible to change the callbacks in elapi_resolve.c to static?
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct in the first patch, rather than non-static in first patch and fixed to static in second.
I agree, but I think I had two sets of changes in the same file: one set for the first patch (changing to static) and another for the second (do not remember). To avoid doing editing and squashing twice I did it once and added on top of the second patch. Is it a bad practice? Seems like a bit of overkill to me to do it twice but I am a novice to the common practices of git so please guide me :-).
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
On 10/05/2009 12:55 PM, Dmitri Pal wrote:
Jakub Hrozek wrote:
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
0001:
- Is it possible to change the callbacks in elapi_resolve.c to static?
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct in the first patch, rather than non-static in first patch and fixed to static in second.
I agree, but I think I had two sets of changes in the same file: one set for the first patch (changing to static) and another for the second (do not remember). To avoid doing editing and squashing twice I did it once and added on top of the second patch. Is it a bad practice? Seems like a bit of overkill to me to do it twice but I am a novice to the common practices of git so please guide me :-).
Yes, it's a bad practice to have unrelated changes in a patch. Most notably because it makes it difficult to apply just one of them if the other isn't ready yet.
There are a couple ways to split patches. The easiest way is probably with 'git add -i' (interactive) which allows you to select individual portions of a diff for including into a commit.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Stephen Gallagher wrote:
On 10/05/2009 12:55 PM, Dmitri Pal wrote:
Jakub Hrozek wrote:
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
0001:
- Is it possible to change the callbacks in elapi_resolve.c to static?
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct in the first patch, rather than non-static in first patch and fixed to static in second.
I agree, but I think I had two sets of changes in the same file: one set for the first patch (changing to static) and another for the second (do not remember). To avoid doing editing and squashing twice I did it once and added on top of the second patch. Is it a bad practice? Seems like a bit of overkill to me to do it twice but I am a novice to the common practices of git so please guide me :-).
Yes, it's a bad practice to have unrelated changes in a patch. Most notably because it makes it difficult to apply just one of them if the other isn't ready yet.
There are a couple ways to split patches. The easiest way is probably with 'git add -i' (interactive) which allows you to select individual portions of a diff for including into a commit.
Ok, point taken.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/05/2009 01:08 PM, Stephen Gallagher wrote:
On 10/05/2009 12:55 PM, Dmitri Pal wrote:
Jakub Hrozek wrote:
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
0001:
- Is it possible to change the callbacks in elapi_resolve.c to static?
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct in the first patch, rather than non-static in first patch and fixed to static in second.
I agree, but I think I had two sets of changes in the same file: one set for the first patch (changing to static) and another for the second (do not remember). To avoid doing editing and squashing twice I did it once and added on top of the second patch. Is it a bad practice? Seems like a bit of overkill to me to do it twice but I am a novice to the common practices of git so please guide me :-).
Yes, it's a bad practice to have unrelated changes in a patch. Most notably because it makes it difficult to apply just one of them if the other isn't ready yet.
There are a couple ways to split patches. The easiest way is probably with 'git add -i' (interactive) which allows you to select individual portions of a diff for including into a commit.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack. sss_getpwnam_done() and friends should return EIO if the sysdb replies with more than one result, I think. EINVAL implies that the fault was with the caller, whereas EIO shows us that something went wrong internally.
I don't like the gratuitous use of discard_const_p in sss_getpwnam_done(). These sync ops are theoretically useful in other places in our code. I don't want people assuming that they can modify the contents of the res->data->name field (for example). They should be talloc_strdup()-ed into the res structure.
sss_useradd.c: ERROR("A user or group with the same name or UID already exists\n"); should be ERROR("A user or group with the same name or ID already exists\n");
tools_util.c: Shouldn't we just make a call to something like a sysdb_getgrent_sync and compare the resulting complete list with the received groups instead of making N complete sync calls, where N could be equal to (or greater than) the total list of groups in the DB?
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
Stephen Gallagher wrote:
On 10/05/2009 01:08 PM, Stephen Gallagher wrote:
On 10/05/2009 12:55 PM, Dmitri Pal wrote:
Jakub Hrozek wrote:
On 10/02/2009 06:49 PM, Dmitri Pal wrote:
Updated versions are attached. Comments are inline.
ACK to all three
> 0001: > * Is it possible to change the callbacks in elapi_resolve.c to
static?
>
Done
Just one thing, it is better to squash the changes to the file that actually introduces the functions, that way you have them correct
in the
first patch, rather than non-static in first patch and fixed to static in second.
I agree, but I think I had two sets of changes in the same file:
one set
for the first patch (changing to static) and another for the second (do not remember). To avoid doing editing and squashing twice I did it once and added on top of the second patch. Is it a bad practice? Seems like a bit of overkill to me to do it twice but I am a novice to the common practices of git so please guide me
:-).
Yes, it's a bad practice to have unrelated changes in a patch. Most notably because it makes it difficult to apply just one of them if the other isn't ready yet.
There are a couple ways to split patches. The easiest way is probably with 'git add -i' (interactive) which allows you to select individual portions of a diff for including into a commit.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack. sss_getpwnam_done() and friends should return EIO if the sysdb replies with more than one result, I think. EINVAL implies that the fault was with the caller, whereas EIO shows us that something went wrong internally.
I don't like the gratuitous use of discard_const_p in sss_getpwnam_done(). These sync ops are theoretically useful in other places in our code. I don't want people assuming that they can modify the contents of the res->data->name field (for example). They should be talloc_strdup()-ed into the res structure.
sss_useradd.c: ERROR("A user or group with the same name or UID already exists\n"); should be ERROR("A user or group with the same name or ID already exists\n");
tools_util.c: Shouldn't we just make a call to something like a sysdb_getgrent_sync and compare the resulting complete list with the received groups instead of making N complete sync calls, where N could be equal to (or greater than) the total list of groups in the DB?
Are you sure you commented to the right patch? You comments seems to be a review of Jakub's patch and this is my patch
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org