Please see the patch comment.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/25/2009 11:00 PM, Dmitri Pal wrote:
Please see the patch comment.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Couple of comments: * Instead of putting libprovider.la into _SOURCES, maybe you wanted to add it to libelapi_la_LIBADD?
* in elapi_print_sink_ctx() instead of casting pointer to uint32_t and printing with %X, you should use %p without casting the pointer
* why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as uint32_t? They seem to have bool semantics, maybe bool would be more appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned char in case of get_bool_config_value() even though it should not matter here as unsigned char will be smaller than 32 bits) return value might not be correct on all platforms. ** the same applies to struct file_prvdr_cfg in file_provider.h, I don't understand using uint32_t to store bool values ** Not strictly related to the patch but why does get_bool_config_value() return char and not bool?
* in elapi_load_lib() why not use snprintf instead of checking the length and then using sprintf() ?
* in elapi_sink_loader() instead of using array of names and a switch just to assign the callback, maybe it would be cleaner to use array of { name, ability } tuples - that way you would just update on one place
* in elapi_sink_create(), if the intent is to have the structure zeroed, why not use calloc()? Or zero the structure out with memset?
Jakub
Jakub Hrozek wrote:
On 08/25/2009 11:00 PM, Dmitri Pal wrote:
Please see the patch comment.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Thank you for the review. My comments are inline.
Couple of comments:
- Instead of putting libprovider.la into _SOURCES, maybe you wanted to
add it to libelapi_la_LIBADD?
I consulted Steve on this one and showed him the makefile. He was Ok with it. But may be you right. I will wait for Steven to take a second look at this one.
- in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
printing with %X, you should use %p without casting the pointer
Most of my C is in pre C99 style. I just did not know about the option. I will definitely switch it to %p if it is portable between platforms. Are you sure it is portable?
- why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
uint32_t? They seem to have bool semantics, maybe bool would be more appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned char in case of get_bool_config_value() even though it should not matter here as unsigned char will be smaller than 32 bits) return value might not be correct on all platforms.
Here is the explanation. I started with "bool". What happened is that when bool is a part of structure it gets aligned on the boundary of the word. In case of 32bit system on the boundary of the 4 bytes. The tree bytes in this case remain uninitialized and valgrind screams and shouts about them. So I decided that since the memory is wasted anyways use 4 bytes and make valgrind happy. This is why I switched from bools to uint32_t. I am not aware of any platform where such assignment will be incorrect. I would prefer not using bools since they make valgrind scream.
** the same applies to struct file_prvdr_cfg in file_provider.h, I don't understand using uint32_t to store bool values
Same reason.
** Not strictly related to the patch but why does get_bool_config_value() return char and not bool?
Because I do not like "bools" and situation with valgrind showed that they can't be trusted. Using unsigned chars and ints is a proven way of expressing bool for years so I do not see a problem continuing along this path.
- in elapi_load_lib() why not use snprintf instead of checking the
length and then using sprintf() ?
Another function that I am not used to use. I also prefer to check first. Why copy strings and figure that they do not fit later if you can check before and see ahead if they fit?
- in elapi_sink_loader() instead of using array of names and a switch
just to assign the callback, maybe it would be cleaner to use array of { name, ability } tuples - that way you would just update on one place
I will open a ticket to improve this part.
- in elapi_sink_create(), if the intent is to have the structure zeroed,
why not use calloc()? Or zero the structure out with memset?
First of all I was told that memset is a slow operation and should be avoided. This is why I initialize only those members that can be cleaned (freed or closed like file descriptor for example).
Also this is another valgrind trick. I noticed that if you use malloc+memset or calloc valgrind does not catch that memory as initialized. So if you later access a member of the structure to check if it is null valgring treats it as an uninitialized read. With the explicit initialization I can see that all the alloc/free logic works correctly by making sure valgrind is happy.
I do not see any of those comments to be critical (unless of cause others do). I would prefer opening tickets to improve different areas and work on them later and take this patch as is. Any concerns or objections?
Jakub
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Dmitri Pal wrote:
Jakub Hrozek wrote:
On 08/25/2009 11:00 PM, Dmitri Pal wrote:
Please see the patch comment.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Thank you for the review. My comments are inline.
Couple of comments:
- Instead of putting libprovider.la into _SOURCES, maybe you wanted to
add it to libelapi_la_LIBADD?
I consulted Steve on this one and showed him the makefile. He was Ok with it. But may be you right. I will wait for Steven to take a second look at this one.
- in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
printing with %X, you should use %p without casting the pointer
Most of my C is in pre C99 style. I just did not know about the option. I will definitely switch it to %p if it is portable between platforms. Are you sure it is portable?
- why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
uint32_t? They seem to have bool semantics, maybe bool would be more appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned char in case of get_bool_config_value() even though it should not matter here as unsigned char will be smaller than 32 bits) return value might not be correct on all platforms.
Here is the explanation. I started with "bool". What happened is that when bool is a part of structure it gets aligned on the boundary of the word. In case of 32bit system on the boundary of the 4 bytes. The tree bytes in this case remain uninitialized and valgrind screams and shouts about them. So I decided that since the memory is wasted anyways use 4 bytes and make valgrind happy. This is why I switched from bools to uint32_t. I am not aware of any platform where such assignment will be incorrect. I would prefer not using bools since they make valgrind scream.
** the same applies to struct file_prvdr_cfg in file_provider.h, I don't understand using uint32_t to store bool values
Same reason.
** Not strictly related to the patch but why does get_bool_config_value() return char and not bool?
Because I do not like "bools" and situation with valgrind showed that they can't be trusted. Using unsigned chars and ints is a proven way of expressing bool for years so I do not see a problem continuing along this path.
- in elapi_load_lib() why not use snprintf instead of checking the
length and then using sprintf() ?
Another function that I am not used to use. I also prefer to check first. Why copy strings and figure that they do not fit later if you can check before and see ahead if they fit?
- in elapi_sink_loader() instead of using array of names and a switch
just to assign the callback, maybe it would be cleaner to use array of { name, ability } tuples - that way you would just update on one place
I will open a ticket to improve this part.
I would have to do some changes in this area anyways in a separate patch because based on on our conversation with Simo I would have to redo the providers anyways a bit. The stderr will be a special variant of the file provider, not a provider by itself. So the code I sent out and the code I am working on now would have to be corrected anyways to allow provider to be used for both cases.
- in elapi_sink_create(), if the intent is to have the structure zeroed,
why not use calloc()? Or zero the structure out with memset?
First of all I was told that memset is a slow operation and should be avoided. This is why I initialize only those members that can be cleaned (freed or closed like file descriptor for example).
Also this is another valgrind trick. I noticed that if you use malloc+memset or calloc valgrind does not catch that memory as initialized. So if you later access a member of the structure to check if it is null valgring treats it as an uninitialized read. With the explicit initialization I can see that all the alloc/free logic works correctly by making sure valgrind is happy.
I do not see any of those comments to be critical (unless of cause others do). I would prefer opening tickets to improve different areas and work on them later and take this patch as is. Any concerns or objections?
Jakub
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Thank you for the review. My comments are inline.
Couple of comments:
- Instead of putting libprovider.la into _SOURCES, maybe you wanted to
add it to libelapi_la_LIBADD?
I consulted Steve on this one and showed him the makefile. He was Ok with it. But may be you right.
Then it's probably right, it's just something that I noticed.
I will wait for Steven to take a second look at this one.
- in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
printing with %X, you should use %p without casting the pointer
Most of my C is in pre C99 style. I just did not know about the option. I will definitely switch it to %p if it is portable between platforms. Are you sure it is portable?
Yes, it is part of C standard.
- why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
uint32_t? They seem to have bool semantics, maybe bool would be more appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned char in case of get_bool_config_value() even though it should not matter here as unsigned char will be smaller than 32 bits) return value might not be correct on all platforms.
Here is the explanation. I started with "bool". What happened is that when bool is a part of structure it gets aligned on the boundary of the word. In case of 32bit system on the boundary of the 4 bytes. The tree bytes in this case remain uninitialized and valgrind screams and shouts about them. So I decided that since the memory is wasted anyways use 4 bytes and make valgrind happy. This is why I switched from bools to uint32_t. I am not aware of any platform where such assignment will be incorrect. I would prefer not using bools since they make valgrind scream.
** the same applies to struct file_prvdr_cfg in file_provider.h, I don't understand using uint32_t to store bool values
Same reason.
** Not strictly related to the patch but why does get_bool_config_value() return char and not bool?
Because I do not like "bools" and situation with valgrind showed that they can't be trusted. Using unsigned chars and ints is a proven way of expressing bool for years so I do not see a problem continuing along this path.
OK, I guess I'm too used to use C99 :-)
- in elapi_load_lib() why not use snprintf instead of checking the
length and then using sprintf() ?
Another function that I am not used to use. I also prefer to check first. Why copy strings and figure that they do not fit later if you can check before and see ahead if they fit?
OK, snprintf is another C99 novelty, so I guess just another way of doing the same.
- in elapi_sink_loader() instead of using array of names and a switch
just to assign the callback, maybe it would be cleaner to use array of { name, ability } tuples - that way you would just update on one place
I will open a ticket to improve this part.
- in elapi_sink_create(), if the intent is to have the structure zeroed,
why not use calloc()? Or zero the structure out with memset?
First of all I was told that memset is a slow operation and should be avoided. This is why I initialize only those members that can be cleaned (freed or closed like file descriptor for example).
Also this is another valgrind trick. I noticed that if you use malloc+memset or calloc valgrind does not catch that memory as initialized. So if you later access a member of the structure to check if it is null valgring treats it as an uninitialized read. With the explicit initialization I can see that all the alloc/free logic works correctly by making sure valgrind is happy.
I do not see any of those comments to be critical (unless of cause others do). I would prefer opening tickets to improve different areas and work on them later and take this patch as is. Any concerns or objections?
No, none of this was critical, more like things I noticed reading the code.
I do not see any of those comments to be critical (unless of cause others do). I would prefer opening tickets to improve different areas and work on them later and take this patch as is. Any concerns or objections?
No, none of this was critical, more like things I noticed reading the code.
Jakub, thanks again!
I tried the make flags you suggested and they worked. I think it looks cleaner as you suggested. So the change will be in the next patch. I also already started eliminating stderr as a separate provider.
I hope to send a new patch later today. It will add new functionality and correct some of the issues you brought up. Are you Ok with this approach or you want me to rework the original patch and then add the new code separately? It would be a tough task but I can try. Or may be you would prefer me squashing them together? Or you would prefer acking this one and treating next patch separately?
One of the things that next patch fixes (that you have not noticed yet (he-he)) is that I use "char *" in many places where it should be a "const char *".
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/28/2009 05:40 AM, Jakub Hrozek wrote:
Thank you for the review. My comments are inline.
Couple of comments:
- Instead of putting libprovider.la into _SOURCES, maybe you wanted to
add it to libelapi_la_LIBADD?
I consulted Steve on this one and showed him the makefile. He was Ok with it. But may be you right.
Then it's probably right, it's just something that I noticed.
While I sincerely appreciate the vote of confidence, I made a mistake. You are correct that is should be part of _LIBADD, not _SOURCES here.
I will wait for Steven to take a second look at this one.
- in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
printing with %X, you should use %p without casting the pointer
Most of my C is in pre C99 style. I just did not know about the option. I will definitely switch it to %p if it is portable between platforms. Are you sure it is portable?
Yes, it is part of C standard.
- why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
uint32_t? They seem to have bool semantics, maybe bool would be more appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned char in case of get_bool_config_value() even though it should not matter here as unsigned char will be smaller than 32 bits) return value might not be correct on all platforms.
Here is the explanation. I started with "bool". What happened is that when bool is a part of structure it gets aligned on the boundary of the word. In case of 32bit system on the boundary of the 4 bytes. The tree bytes in this case remain uninitialized and valgrind screams and shouts about them. So I decided that since the memory is wasted anyways use 4 bytes and make valgrind happy. This is why I switched from bools to uint32_t.
In order to waste less memory, wouldn't a uint8_t be a better choice?
I am not aware of any platform where such assignment will be incorrect. I would prefer not using bools since they make valgrind scream.
** the same applies to struct file_prvdr_cfg in file_provider.h, I don't understand using uint32_t to store bool values
Same reason.
** Not strictly related to the patch but why does get_bool_config_value() return char and not bool?
Because I do not like "bools" and situation with valgrind showed that they can't be trusted. Using unsigned chars and ints is a proven way of expressing bool for years so I do not see a problem continuing along this path.
OK, I guess I'm too used to use C99 :-)
- in elapi_load_lib() why not use snprintf instead of checking the
length and then using sprintf() ?
Another function that I am not used to use. I also prefer to check first. Why copy strings and figure that they do not fit later if you can check before and see ahead if they fit?
OK, snprintf is another C99 novelty, so I guess just another way of doing the same.
- in elapi_sink_loader() instead of using array of names and a switch
just to assign the callback, maybe it would be cleaner to use array of { name, ability } tuples - that way you would just update on one place
I will open a ticket to improve this part.
- in elapi_sink_create(), if the intent is to have the structure zeroed,
why not use calloc()? Or zero the structure out with memset?
First of all I was told that memset is a slow operation and should be avoided. This is why I initialize only those members that can be cleaned (freed or closed like file descriptor for example).
memset() is a bad operation to use when dealing with strings/null-terminated arrays. It's perfectly valid to use on structs (and advisable, even). The reason it's bad for strings is that it does two things unnecessarily: 1) it modifies every entry in the array when it needs only set the first entry to null-termination and 2) it gives a false sense of security that the string is now guaranteed to be terminated (which is not the case if you write past the end of the buffer). On the other hand, its use in a struct is completely reasonable because (unless there are static buffers in the struct) you are only writing a minimal set of zeroes.
Also this is another valgrind trick. I noticed that if you use malloc+memset or calloc valgrind does not catch that memory as initialized. So if you later access a member of the structure to check if it is null valgring treats it as an uninitialized read. With the explicit initialization I can see that all the alloc/free logic works correctly by making sure valgrind is happy.
I do not see any of those comments to be critical (unless of cause others do). I would prefer opening tickets to improve different areas and work on them later and take this patch as is. Any concerns or objections?
No, none of this was critical, more like things I noticed reading the code.
_______________________________________________ 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/
Stephen Gallagher wrote:
On 08/28/2009 05:40 AM, Jakub Hrozek wrote:
Thank you for the review. My comments are inline.
Couple of comments:
- Instead of putting libprovider.la into _SOURCES, maybe you wanted to
add it to libelapi_la_LIBADD?
I consulted Steve on this one and showed him the makefile. He was Ok with it. But may be you right.
Then it's probably right, it's just something that I noticed.
While I sincerely appreciate the vote of confidence, I made a mistake. You are correct that is should be part of _LIBADD, not _SOURCES here.
Well next patch will correct it. I already have it the code
I will wait for Steven to take a second look at this one.
- in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
printing with %X, you should use %p without casting the pointer
Most of my C is in pre C99 style. I just did not know about the
option.
I will definitely switch it to %p if it is portable between platforms. Are you sure it is portable?
Yes, it is part of C standard.
- why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
uint32_t? They seem to have bool semantics, maybe bool would be more appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned char in case of get_bool_config_value() even though it should not
matter
here as unsigned char will be smaller than 32 bits) return value might not be correct on all platforms.
Here is the explanation. I started with "bool". What happened is that when bool is a part of structure it gets aligned on the boundary of the word. In case of 32bit system on the boundary of the 4 bytes. The tree bytes in this case remain uninitialized and valgrind screams and shouts about them. So I decided that since the memory is wasted anyways use 4 bytes and make valgrind happy. This is why I switched from bools to uint32_t.
In order to waste less memory, wouldn't a uint8_t be a better choice?
It is the structure that would be allocated once per instance of ELAPI. I do not see it being a big deal. And the compiler would try to align things in the struct anyways. I will keep it in mind in cases where memory is allocated frequently and large volumes.
[snip]
memset() is a bad operation to use when dealing with strings/null-terminated arrays. It's perfectly valid to use on structs (and advisable, even). The reason it's bad for strings is that it does two things unnecessarily: 1) it modifies every entry in the array when it needs only set the first entry to null-termination and 2) it gives a false sense of security that the string is now guaranteed to be terminated (which is not the case if you write past the end of the buffer). On the other hand, its use in a struct is completely reasonable because (unless there are static buffers in the struct) you are only writing a minimal set of zeroes.
I will check with valgrind if my observation about memset and calloc is false I will revert to those where it makes sense.
I think it makes sense to wait for the next patch and review the second one first and then push them together.
Thanks, Dmitri
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/28/2009 11:40 AM, Jakub Hrozek wrote:
No, none of this was critical, more like things I noticed reading the code.
FWIW, while reviewing the "ELAPI Adding file provider and CSV format: patch, I noticed that my comments were taken into account.
I also didn't give a formal ack yet, so:
ACK
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/07/2009 08:05 AM, Jakub Hrozek wrote:
On 08/28/2009 11:40 AM, Jakub Hrozek wrote:
No, none of this was critical, more like things I noticed reading the code.
FWIW, while reviewing the "ELAPI Adding file provider and CSV format: patch, I noticed that my comments were taken into account.
I also didn't give a formal ack yet, so:
ACK
Jakub
As per another comment in this thread, I am not going to push this patch until the "ELAPI Adding file provider and CSV" format patch is finished. _______________________________________________ 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/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/08/2009 06:47 AM, Stephen Gallagher wrote:
On 09/07/2009 08:05 AM, Jakub Hrozek wrote:
On 08/28/2009 11:40 AM, Jakub Hrozek wrote:
No, none of this was critical, more like things I noticed reading the code.
FWIW, while reviewing the "ELAPI Adding file provider and CSV format: patch, I noticed that my comments were taken into account.
I also didn't give a formal ack yet, so:
ACK
Jakub
As per another comment in this thread, I am not going to push this patch until the "ELAPI Adding file provider and CSV" format patch is finished. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
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/
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Stephen Gallagher