Currently, SSSD only supports using libunistring to manage unicode strings. There are some platforms out there (such as RHEL 5) that do not have libunistring available. With this patch, we add an optional flag to autoconf to allow SSSD to link against Glib and use its unicode functionality.
----- Original Message -----
Currently, SSSD only supports using libunistring to manage unicode strings. There are some platforms out there (such as RHEL 5) that do not have libunistring available. With this patch, we add an optional flag to autoconf to allow SSSD to link against Glib and use its unicode functionality.
Two quick comments. 1. Can you use a shorter function name than sss_utf8_case_insensitive_equality ? I am all for clarity, but when the function name is so long arguments go on the next line w/o any nesting it is bad :-)
2. given result returns just a boolean I would prefer to not use it and just use ret. 0 is 'matches' and some other error code means it didn't match, something like ERANGE could be used. or if you feel strongly about having your own value define a value like -1 as ENOMATCH earlier in the code. It makes for a more usable interface if you do not have to care about 2 return variables IMO.
Simo.
On Fri, 2011-12-02 at 15:16 -0500, Simo Sorce wrote:
----- Original Message -----
Currently, SSSD only supports using libunistring to manage unicode strings. There are some platforms out there (such as RHEL 5) that do not have libunistring available. With this patch, we add an optional flag to autoconf to allow SSSD to link against Glib and use its unicode functionality.
Two quick comments.
- Can you use a shorter function name than sss_utf8_case_insensitive_equality ?
I am all for clarity, but when the function name is so long arguments go on the next line w/o any nesting it is bad :-)
- given result returns just a boolean I would prefer to not use it and just use ret.
0 is 'matches' and some other error code means it didn't match, something like ERANGE could be used. or if you feel strongly about having your own value define a value like -1 as ENOMATCH earlier in the code. It makes for a more usable interface if you do not have to care about 2 return variables IMO.
Thanks for the review. Revised patch is attached.
I opted to use ENOTUNIQ for the "not matched" response, since it should be impossible to receive that error from any of the underlying functions within the comparison.
On Mon, 2011-12-05 at 09:30 -0500, Stephen Gallagher wrote:
On Fri, 2011-12-02 at 15:16 -0500, Simo Sorce wrote:
----- Original Message -----
Currently, SSSD only supports using libunistring to manage unicode strings. There are some platforms out there (such as RHEL 5) that do not have libunistring available. With this patch, we add an optional flag to autoconf to allow SSSD to link against Glib and use its unicode functionality.
Two quick comments.
- Can you use a shorter function name than sss_utf8_case_insensitive_equality ?
I am all for clarity, but when the function name is so long arguments go on the next line w/o any nesting it is bad :-)
- given result returns just a boolean I would prefer to not use it and just use ret.
0 is 'matches' and some other error code means it didn't match, something like ERANGE could be used. or if you feel strongly about having your own value define a value like -1 as ENOMATCH earlier in the code. It makes for a more usable interface if you do not have to care about 2 return variables IMO.
Thanks for the review. Revised patch is attached.
I opted to use ENOTUNIQ for the "not matched" response, since it should be impossible to receive that error from any of the underlying functions within the comparison.
Simo nacked off-list. I've added a new error code ENOMATCH (-1) and adjusted the #ifdef blocks to surround the functions completely (for readability).
New patch attached.
On Mon, 2011-12-05 at 11:06 -0500, Stephen Gallagher wrote:
On Mon, 2011-12-05 at 09:30 -0500, Stephen Gallagher wrote:
On Fri, 2011-12-02 at 15:16 -0500, Simo Sorce wrote:
----- Original Message -----
Currently, SSSD only supports using libunistring to manage unicode strings. There are some platforms out there (such as RHEL 5) that do not have libunistring available. With this patch, we add an optional flag to autoconf to allow SSSD to link against Glib and use its unicode functionality.
Two quick comments.
- Can you use a shorter function name than sss_utf8_case_insensitive_equality ?
I am all for clarity, but when the function name is so long arguments go on the next line w/o any nesting it is bad :-)
- given result returns just a boolean I would prefer to not use it and just use ret.
0 is 'matches' and some other error code means it didn't match, something like ERANGE could be used. or if you feel strongly about having your own value define a value like -1 as ENOMATCH earlier in the code. It makes for a more usable interface if you do not have to care about 2 return variables IMO.
Thanks for the review. Revised patch is attached.
I opted to use ENOTUNIQ for the "not matched" response, since it should be impossible to receive that error from any of the underlying functions within the comparison.
Simo nacked off-list. I've added a new error code ENOMATCH (-1) and adjusted the #ifdef blocks to surround the functions completely (for readability).
New patch attached.
Unfortunately I have no time to actually test the patch, but it's an ACK from me on every other count.
Simo.
On Mon, 2011-12-05 at 11:09 -0500, Simo Sorce wrote:
On Mon, 2011-12-05 at 11:06 -0500, Stephen Gallagher wrote:
On Mon, 2011-12-05 at 09:30 -0500, Stephen Gallagher wrote:
On Fri, 2011-12-02 at 15:16 -0500, Simo Sorce wrote:
----- Original Message -----
Currently, SSSD only supports using libunistring to manage unicode strings. There are some platforms out there (such as RHEL 5) that do not have libunistring available. With this patch, we add an optional flag to autoconf to allow SSSD to link against Glib and use its unicode functionality.
Two quick comments.
- Can you use a shorter function name than sss_utf8_case_insensitive_equality ?
I am all for clarity, but when the function name is so long arguments go on the next line w/o any nesting it is bad :-)
- given result returns just a boolean I would prefer to not use it and just use ret.
0 is 'matches' and some other error code means it didn't match, something like ERANGE could be used. or if you feel strongly about having your own value define a value like -1 as ENOMATCH earlier in the code. It makes for a more usable interface if you do not have to care about 2 return variables IMO.
Thanks for the review. Revised patch is attached.
I opted to use ENOTUNIQ for the "not matched" response, since it should be impossible to receive that error from any of the underlying functions within the comparison.
Simo nacked off-list. I've added a new error code ENOMATCH (-1) and adjusted the #ifdef blocks to surround the functions completely (for readability).
New patch attached.
Unfortunately I have no time to actually test the patch, but it's an ACK from me on every other count.
I've been testing it locally since last week, along with the test tools I created for verifying the original bugs around UTF8 support.
Pushed to master and sssd-1-6.
Backported for sssd-1-5 (makefile changes).
sssd-devel@lists.fedorahosted.org
-
Simo Sorce -
Simo Sorce -
Stephen Gallagher