<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" &lt;mzidek@redhat.com&gt;<br><b>To: </b>"Development of the System Security Services Daemon" &lt;sssd-devel@lists.fedorahosted.org&gt;<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>&gt; On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:<br>&gt;&gt; ehlo,<br>&gt;&gt;<br>&gt;&gt; This patch add possibility to replace whitespace in user and group<br>&gt;&gt; names with<br>&gt;&gt; a specified string. With string "-", sssd will return the same result as<br>&gt;&gt; winbind enabled option "winbind normalize names"<br>&gt;&gt;<br>&gt;&gt; Resolves:<br>&gt;&gt; https://fedorahosted.org/sssd/ticket/1854<br>&gt;&gt;<br>&gt;&gt; patch is attached.<br>&gt;&gt;<br>&gt;&gt; LS<br>&gt;&gt;<br>&gt;<br>&gt; Hi Lukas,<br>&gt;<br>&gt; src/config/SSSDConfig/__init__.py.in<br>&gt; 76 &nbsp; &nbsp; 'whitespaces_replacement_string': _('All white spaces will be<br>&gt; replaced in group name with this string'),<br>&gt;<br>&gt; Not just group name, but also user name. IMO something<br>&gt; like this would be better:<br>&gt; "All white spaces in group or user names will be replaced with<br>&gt; this string." Or something similar.<br>&gt;<br>&gt; Also please put changes in<br>&gt; src/config/SSSDConfig/__init__.py.in and man page changes into a<br>&gt; separate patch. We will than ping a native speaker (Stephen ?)<br>&gt; to check the wording when the coding part is accepted.<br>&gt;<br>&gt; The option name could be at least a little shorter, maybe:<br>&gt; replace_whitespace_str<br>&gt; or<br>&gt; replace_spaces_by<br>&gt;<br>&gt; But this is not a strong requirement, if you do not like the above<br>&gt; names and can't come up with something better, we can stick to the<br>&gt; current name.<br>&gt;<br>&gt; &nbsp; &nbsp;35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,<br>&gt; &nbsp; &nbsp;... &lt;snip&gt;<br>&gt; &nbsp; &nbsp;76 &nbsp; &nbsp; while (*ptr != '\0') {<br>&gt; &nbsp; &nbsp;77 &nbsp; &nbsp; &nbsp; &nbsp; if (isspace(*ptr)){<br>&gt; &nbsp; &nbsp;78 &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; new_name = talloc_asprintf_append(...<br>&gt; &nbsp; &nbsp;79 &nbsp; &nbsp; &nbsp; &nbsp; } else{<br>&gt; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; ^^^^^<br>&gt; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;Missing space after else<br>&gt; &nbsp; &nbsp;80 &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; new_name = talloc_asprintf_append(...<br>&gt; &nbsp; &nbsp;81 &nbsp; &nbsp; &nbsp; &nbsp; }<br>&gt;<br>&gt; The patch seem to work fine. But functions like<br>&gt; replace_whitespaces and the reverse version can be<br>&gt; easily unit-tested. It would be great if you could<br>&gt; add unit tests for these two.<br>&gt;<br>&gt; Michal<br><br>Also when you call the new functions, you use this pattern<br>&nbsp;&nbsp;887 &nbsp; &nbsp; &nbsp; &nbsp; name = reverse_replace_withespaces(dctx, name,<br>&nbsp;&nbsp;888<br>&nbsp;&nbsp;889 &nbsp; &nbsp; &nbsp; &nbsp; if (name == NULL) {<br>&nbsp;&nbsp;890 &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; DEBUG(SSSDBG_CRIT_FAILURE,<br>&nbsp;&nbsp;891 &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; "reverse_replace_withespaces...<br>&nbsp;&nbsp;892 &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; continue;<br>&nbsp;&nbsp;893 &nbsp; &nbsp; &nbsp; &nbsp; }<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>