<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'>Was talking to Lukas about the name and came up with this suggestion, so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?<br><br>Dan<br><br><hr id="zwchr"><div style="color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Michal Židek" <mzidek@redhat.com><br><b>To: </b>"Development of the System Security Services Daemon" <sssd-devel@lists.fedorahosted.org><br><b>Sent: </b>Thursday, July 24, 2014 12:12:49 PM<br><b>Subject: </b>Re: [SSSD] [PATCH] NSS: replace with space with specified character in names.<br><br>On 07/24/2014 05:53 PM, Michal Židek wrote:<br>> On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:<br>>> ehlo,<br>>><br>>> This patch add possibility to replace whitespace in user and group<br>>> names with<br>>> a specified string. With string "-", sssd will return the same result as<br>>> winbind enabled option "winbind normalize names"<br>>><br>>> Resolves:<br>>> https://fedorahosted.org/sssd/ticket/1854<br>>><br>>> patch is attached.<br>>><br>>> LS<br>>><br>><br>> Hi Lukas,<br>><br>> src/config/SSSDConfig/__init__.py.in<br>> 76 'whitespaces_replacement_string': _('All white spaces will be<br>> replaced in group name with this string'),<br>><br>> Not just group name, but also user name. IMO something<br>> like this would be better:<br>> "All white spaces in group or user names will be replaced with<br>> this string." Or something similar.<br>><br>> Also please put changes in<br>> src/config/SSSDConfig/__init__.py.in and man page changes into a<br>> separate patch. We will than ping a native speaker (Stephen ?)<br>> to check the wording when the coding part is accepted.<br>><br>> The option name could be at least a little shorter, maybe:<br>> replace_whitespace_str<br>> or<br>> replace_spaces_by<br>><br>> But this is not a strong requirement, if you do not like the above<br>> names and can't come up with something better, we can stick to the<br>> current name.<br>><br>> 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,<br>> ... <snip><br>> 76 while (*ptr != '\0') {<br>> 77 if (isspace(*ptr)){<br>> 78 new_name = talloc_asprintf_append(...<br>> 79 } else{<br>> ^^^^^<br>> Missing space after else<br>> 80 new_name = talloc_asprintf_append(...<br>> 81 }<br>><br>> The patch seem to work fine. But functions like<br>> replace_whitespaces and the reverse version can be<br>> easily unit-tested. It would be great if you could<br>> add unit tests for these two.<br>><br>> Michal<br><br>Also when you call the new functions, you use this pattern<br> 887 name = reverse_replace_withespaces(dctx, name,<br> 888<br> 889 if (name == NULL) {<br> 890 DEBUG(SSSDBG_CRIT_FAILURE,<br> 891 "reverse_replace_withespaces...<br> 892 continue;<br> 893 }<br><br>Other functions in this part set ret and jump to done<br>if something goes wrong. We could do the same here.<br><br>Michal<br>_______________________________________________<br>sssd-devel mailing list<br>sssd-devel@lists.fedorahosted.org<br>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel<br></div><br></div></body></html>