[PATCH] NSS: replace with space with specified character in names.

[Lukas Slebodnik]

ehlo,

This patch add possibility to replace whitespace in user and group names with
a specified string. With string "-", sssd will return the same result as
winbind enabled option "winbind normalize names"

Resolves:
https://fedorahosted.org/sssd/ticket/1854

patch is attached.

LS

[Michal Židek]

On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> ehlo,
>
> This patch add possibility to replace whitespace in user and group names with
> a specified string. With string "-", sssd will return the same result as
> winbind enabled option "winbind normalize names"
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1854
>
> patch is attached.
>
> LS

Hi Lukas,

src/config/SSSDConfig/__init__.py.in
76     'whitespaces_replacement_string': _('All white spaces will be 
replaced in group name with this string'),

Not just group name, but also user name. IMO something
like this would be better:
"All white spaces in group or user names will be replaced with
this string." Or something similar.

Also please put changes in
src/config/SSSDConfig/__init__.py.in and man page changes into a
separate patch. We will than ping a native speaker (Stephen ?)
to check the wording when the coding part is accepted.

The option name could be at least a little shorter, maybe:
replace_whitespace_str
or
replace_spaces_by

But this is not a strong requirement, if you do not like the above
names and can't come up with something better, we can stick to the
current name.

35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
   ... <snip>
   76     while (*ptr != '\0') {
   77         if (isspace(*ptr)){
   78             new_name = talloc_asprintf_append(...
   79         } else{
                ^^^^^
                   Missing space after else
   80             new_name = talloc_asprintf_append(...
   81         }

The patch seem to work fine. But functions like
replace_whitespaces and the reverse version can be
easily unit-tested. It would be great if you could
add unit tests for these two.

Michal

[Michal Židek]

On 07/24/2014 05:53 PM, Michal Židek wrote:
> On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > ehlo,
> >
> > This patch add possibility to replace whitespace in user and group
> > names with
> > a specified string. With string "-", sssd will return the same result as
> > winbind enabled option "winbind normalize names"
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1854
> >
> > patch is attached.
> >
> > LS
>
> Hi Lukas,
>
> src/config/SSSDConfig/__init__.py.in
> 76     'whitespaces_replacement_string': _('All white spaces will be
> replaced in group name with this string'),
>
> Not just group name, but also user name. IMO something
> like this would be better:
> "All white spaces in group or user names will be replaced with
> this string." Or something similar.
>
> Also please put changes in
> src/config/SSSDConfig/__init__.py.in and man page changes into a
> separate patch. We will than ping a native speaker (Stephen ?)
> to check the wording when the coding part is accepted.
>
> The option name could be at least a little shorter, maybe:
> replace_whitespace_str
> or
> replace_spaces_by
>
> But this is not a strong requirement, if you do not like the above
> names and can't come up with something better, we can stick to the
> current name.
>
> 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
>    ... <snip>
>    76     while (*ptr != '\0') {
>    77         if (isspace(*ptr)){
>    78             new_name = talloc_asprintf_append(...
>    79         } else{
>                 ^^^^^
>                    Missing space after else
>    80             new_name = talloc_asprintf_append(...
>    81         }
>
> The patch seem to work fine. But functions like
> replace_whitespaces and the reverse version can be
> easily unit-tested. It would be great if you could
> add unit tests for these two.
>
> Michal

Also when you call the new functions, you use this pattern
  887         name = reverse_replace_withespaces(dctx, name,
  888
  889         if (name == NULL) {
  890             DEBUG(SSSDBG_CRIT_FAILURE,
  891                   "reverse_replace_withespaces...
  892             continue;
  893         }

Other functions in this part set ret and jump to done
if something goes wrong. We could do the same here.

Michal

[Dan Lavu]

Was talking to Lukas about the name and came up with this suggestion, so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?

Dan

----- Original Message -----

From: "Michal Židek"  
To: "Development of the System Security Services Daemon"  
Sent: Thursday, July 24, 2014 12:12:49 PM 
Subject: Re: [SSSD] [PATCH] NSS: replace with space with specified character in names.

On 07/24/2014 05:53 PM, Michal Židek wrote:
> On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > ehlo,
> >
> > This patch add possibility to replace whitespace in user and group 
> > names with 
> > a specified string. With string "-", sssd will return the same result as 
> > winbind enabled option "winbind normalize names"
> >
> > Resolves: 
> > https://fedorahosted.org/sssd/ticket/1854
> >
> > patch is attached.
> >
> > LS
>
> Hi Lukas,
>
> src/config/SSSDConfig/__init__.py.in 
> 76 'whitespaces_replacement_string': _('All white spaces will be 
> replaced in group name with this string'),
>
> Not just group name, but also user name. IMO something 
> like this would be better: 
> "All white spaces in group or user names will be replaced with 
> this string." Or something similar.
>
> Also please put changes in 
> src/config/SSSDConfig/__init__.py.in and man page changes into a 
> separate patch. We will than ping a native speaker (Stephen ?) 
> to check the wording when the coding part is accepted.
>
> The option name could be at least a little shorter, maybe: 
> replace_whitespace_str 
> or 
> replace_spaces_by
>
> But this is not a strong requirement, if you do not like the above 
> names and can't come up with something better, we can stick to the 
> current name.
>
> 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx, 
> ... <snip> 
> 76 while (*ptr != '\0') { 
> 77 if (isspace(*ptr)){ 
> 78 new_name = talloc_asprintf_append(... 
> 79 } else{ 
> ^^^^^ 
> Missing space after else 
> 80 new_name = talloc_asprintf_append(... 
> 81 }
>
> The patch seem to work fine. But functions like 
> replace_whitespaces and the reverse version can be 
> easily unit-tested. It would be great if you could 
> add unit tests for these two.
>
> Michal

Also when you call the new functions, you use this pattern 
887 name = reverse_replace_withespaces(dctx, name, 
888 
889 if (name == NULL) { 
890 DEBUG(SSSDBG_CRIT_FAILURE, 
891 "reverse_replace_withespaces... 
892 continue; 
893 }

Other functions in this part set ret and jump to done 
if something goes wrong. We could do the same here.

Michal 
_______________________________________________ 
sssd-devel mailing list 
[e-mail] 
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> Was talking to Lukas about the name and came up with this suggestion, so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?

I like it!

Dan, given you're a native speaker, would you mind taking a look at the
changes to src/man/sssd.conf.5.xml ?

[Dan Lavu]

Of course.

----- Original Message -----

From: "Jakub Hrozek"  
To: [e-mail] 
Sent: Thursday, July 24, 2014 3:18:15 PM 
Subject: Re: [SSSD] [PATCH] NSS: replace with space with specified character in names.

On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> Was talking to Lukas about the name and came up with this suggestion, so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?

I like it!

Dan, given you're a native speaker, would you mind taking a look at the 
changes to src/man/sssd.conf.5.xml ? 
_______________________________________________ 
sssd-devel mailing list 
[e-mail] 
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Simo Sorce]

On Thu, 2014-07-24 at 21:18 +0200, Jakub Hrozek wrote:
> On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> > Was talking to Lukas about the name and came up with this suggestion, so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
>
> I like it!
>
> Dan, given you're a native speaker, would you mind taking a look at the
> changes to src/man/sssd.conf.5.xml ?

whitespace_substitute would be shorter.

Simo.

[Lukas Slebodnik]

On (28/07/14 10:14), Simo Sorce wrote:
> On Thu, 2014-07-24 at 21:18 +0200, Jakub Hrozek wrote:
> > On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> > > Was talking to Lukas about the name and came up with this suggestion,
> > > so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
> >
> > I like it!
> >
> > Dan, given you're a native speaker, would you mind taking a look at the
> > changes to src/man/sssd.conf.5.xml ?
>
> whitespace_substitute would be shorter.
>
> Simo.

If others agree, we can rename this configuration option. sssd was not
released officially with "override_default_whitespace" yet.

LS

[Jakub Hrozek]

On Tue, Aug 05, 2014 at 03:39:11PM +0200, Lukas Slebodnik wrote:
> On (28/07/14 10:14), Simo Sorce wrote:
> > On Thu, 2014-07-24 at 21:18 +0200, Jakub Hrozek wrote:
> > > On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> > > > Was talking to Lukas about the name and came up with this suggestion,
> > > > so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
> > >
> > > I like it!
> > >
> > > Dan, given you're a native speaker, would you mind taking a look at the
> > > changes to src/man/sssd.conf.5.xml ?
> >
> > whitespace_substitute would be shorter.
> >
> > Simo.
>
> If others agree, we can rename this configuration option. sssd was not
> released officially with "override_default_whitespace" yet.

I would prefer to first discuss if we should replace all whitespace or
just "space". See:
    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=case

I'm sorry I didn't catch this earlier, only when I started working on
extending the functionality to other responders.

I personally favor safety and simplicity over functionality, so I think
it would be better to only replace space (as winbind does) to be able to
replace symetrically. I also realize the RFE talked about "whitespace"
but I believe that was a misunderstanding. I reached out to the original
reporter of the RFE for clarification..

[Pavel Březina]

On 08/05/2014 03:56 PM, Jakub Hrozek wrote:
> On Tue, Aug 05, 2014 at 03:39:11PM +0200, Lukas Slebodnik wrote:
> > On (28/07/14 10:14), Simo Sorce wrote:
> > > On Thu, 2014-07-24 at 21:18 +0200, Jakub Hrozek wrote:
> > > > On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> > > > > Was talking to Lukas about the name and came up with this suggestion,
> > > > > so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
> > > >
> > > > I like it!
> > > >
> > > > Dan, given you're a native speaker, would you mind taking a look at the
> > > > changes to src/man/sssd.conf.5.xml ?
> > >
> > > whitespace_substitute would be shorter.
> > >
> > > Simo.
> >
> > If others agree, we can rename this configuration option. sssd was not
> > released officially with "override_default_whitespace" yet.
>
> I would prefer to first discuss if we should replace all whitespace or
> just "space". See:
>      https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=case
>
> I'm sorry I didn't catch this earlier, only when I started working on
> extending the functionality to other responders.
>
> I personally favor safety and simplicity over functionality, so I think
> it would be better to only replace space (as winbind does) to be able to
> replace symetrically. I also realize the RFE talked about "whitespace"
> but I believe that was a misunderstanding. I reached out to the original
> reporter of the RFE for clarification..

I agree with replacing only space, unless there's an RFE. If you ask me 
it wasn't really necessary to implement string as replacement, a 
character was enough :-)

I'm looking into the code right now... it doesn't seem to cope with an 
input that also contains the replacement character. To make it really 
reversible we should first search the input for the replacement 
character and replace it with an escape sequence.

Sorry, I did not follow the whole thread so maybe it was already mentioned?

[Jakub Hrozek]

On Wed, Aug 06, 2014 at 11:05:50AM +0200, Pavel Březina wrote:
> On 08/05/2014 03:56 PM, Jakub Hrozek wrote:
> > On Tue, Aug 05, 2014 at 03:39:11PM +0200, Lukas Slebodnik wrote:
> > > On (28/07/14 10:14), Simo Sorce wrote:
> > > > On Thu, 2014-07-24 at 21:18 +0200, Jakub Hrozek wrote:
> > > > > On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> > > > > > Was talking to Lukas about the name and came up with this suggestion,
> > > > > > so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
> > > > >
> > > > > I like it!
> > > > >
> > > > > Dan, given you're a native speaker, would you mind taking a look at the
> > > > > changes to src/man/sssd.conf.5.xml ?
> > > >
> > > > whitespace_substitute would be shorter.
> > > >
> > > > Simo.
> > >
> > > If others agree, we can rename this configuration option. sssd was not
> > > released officially with "override_default_whitespace" yet.
> >
> > I would prefer to first discuss if we should replace all whitespace or
> > just "space". See:
> >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=case
> >
> > I'm sorry I didn't catch this earlier, only when I started working on
> > extending the functionality to other responders.
> >
> > I personally favor safety and simplicity over functionality, so I think
> > it would be better to only replace space (as winbind does) to be able to
> > replace symetrically. I also realize the RFE talked about "whitespace"
> > but I believe that was a misunderstanding. I reached out to the original
> > reporter of the RFE for clarification..
>
> I agree with replacing only space, unless there's an RFE. If you ask me it
> wasn't really necessary to implement string as replacement, a character was
> enough :-)
>
> I'm looking into the code right now... it doesn't seem to cope with an input
> that also contains the replacement character. To make it really reversible
> we should first search the input for the replacement character and replace
> it with an escape sequence.
>
> Sorry, I did not follow the whole thread so maybe it was already mentioned?

No, it wasn't.

I'm working on the patches right now, I'll be happy to work with you
during patch review on straightening all the corner cases.

btw a confirmation from the customer who requested the feature arrived
tonoght -- they really want to just replace the space. The "white"
prefix was added when our support were turning the customer's request
into a Bugzilla report :-)

[Lukas Slebodnik]

On (06/08/14 11:05), Pavel Březina wrote:
> On 08/05/2014 03:56 PM, Jakub Hrozek wrote:
> > On Tue, Aug 05, 2014 at 03:39:11PM +0200, Lukas Slebodnik wrote:
> > > On (28/07/14 10:14), Simo Sorce wrote:
> > > > On Thu, 2014-07-24 at 21:18 +0200, Jakub Hrozek wrote:
> > > > > On Thu, Jul 24, 2014 at 03:20:45PM -0400, Dan Lavu wrote:
> > > > > > Was talking to Lukas about the name and came up with this suggestion,
> > > > > > so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
> > > > >
> > > > > I like it!
> > > > >
> > > > > Dan, given you're a native speaker, would you mind taking a look at the
> > > > > changes to src/man/sssd.conf.5.xml ?
> > > >
> > > > whitespace_substitute would be shorter.
> > > >
> > > > Simo.
> > >
> > > If others agree, we can rename this configuration option. sssd was not
> > > released officially with "override_default_whitespace" yet.
> >
> > I would prefer to first discuss if we should replace all whitespace or
> > just "space". See:
> >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=case
> >
> > I'm sorry I didn't catch this earlier, only when I started working on
> > extending the functionality to other responders.
> >
> > I personally favor safety and simplicity over functionality, so I think
> > it would be better to only replace space (as winbind does) to be able to
> > replace symetrically. I also realize the RFE talked about "whitespace"
> > but I believe that was a misunderstanding. I reached out to the original
> > reporter of the RFE for clarification..
>
> I agree with replacing only space, unless there's an RFE. If you ask me it
> wasn't really necessary to implement string as replacement, a character was
> enough

I was not sure about this. So I decided to write more general version.
We don't have valiadation of configuration options; the best solution will be
to update manual page and mention that just the 1st character will be used.
> I'm looking into the code right now... it doesn't seem to cope with an input
> that also contains the replacement character.
> To make it really reversible we
> should first search the input for the replacement character and replace it
> with an escape sequence.

There is a unit test, which test replacement spaces in string with space.

I don't think we need to escape it. Windbind does not have this character
configurable and they should escape replacement string in original string.
We can use different string(character).
> Sorry, I did not follow the whole thread so maybe it was already mentioned?

It is better to write comment after a commit then never :-)

LS

[Lukas Slebodnik]

On (24/07/14 17:53), Michal Židek wrote:
> On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > ehlo,
> >
> > This patch add possibility to replace whitespace in user and group names with
> > a specified string. With string "-", sssd will return the same result as
> > winbind enabled option "winbind normalize names"
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1854
> >
> > patch is attached.
> >
> > LS
>
> Hi Lukas,
>
> src/config/SSSDConfig/__init__.py.in
> 76     'whitespaces_replacement_string': _('All white spaces will be replaced
> in group name with this string'),
>
> Not just group name, but also user name. IMO something
> like this would be better:
> "All white spaces in group or user names will be replaced with
> this string." Or something similar.
>
> Also please put changes in
> src/config/SSSDConfig/__init__.py.in and man page changes into a
> separate patch. We will than ping a native speaker (Stephen ?)
> to check the wording when the coding part is accepted.
>
> The option name could be at least a little shorter, maybe:
> replace_whitespace_str
> or
> replace_spaces_by
>
> But this is not a strong requirement, if you do not like the above
> names and can't come up with something better, we can stick to the
> current name.
>
> 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
>  ... <snip>
>  76     while (*ptr != '\0') {
>  77         if (isspace(*ptr)){
>  78             new_name = talloc_asprintf_append(...
>  79         } else{
>               ^^^^^
>                  Missing space after else
>  80             new_name = talloc_asprintf_append(...
>  81         }
>
> The patch seem to work fine. But functions like
> replace_whitespaces and the reverse version can be
> easily unit-tested. It would be great if you could
> add unit tests for these two.

Attached patches fix also comments from other mails in this thread.

LS

[Michal Židek]

On 07/25/2014 01:42 PM, Lukas Slebodnik wrote:
> On (24/07/14 17:53), Michal Židek wrote:
> > On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > > ehlo,
> > >
> > > This patch add possibility to replace whitespace in user and group names with
> > > a specified string. With string "-", sssd will return the same result as
> > > winbind enabled option "winbind normalize names"
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/1854
> > >
> > > patch is attached.
> > >
> > > LS
> >
> > Hi Lukas,
> >
> > src/config/SSSDConfig/__init__.py.in
> > 76     'whitespaces_replacement_string': _('All white spaces will be replaced
> > in group name with this string'),
> >
> > Not just group name, but also user name. IMO something
> > like this would be better:
> > "All white spaces in group or user names will be replaced with
> > this string." Or something similar.
> >
> > Also please put changes in
> > src/config/SSSDConfig/__init__.py.in and man page changes into a
> > separate patch. We will than ping a native speaker (Stephen ?)
> > to check the wording when the coding part is accepted.
> >
> > The option name could be at least a little shorter, maybe:
> > replace_whitespace_str
> > or
> > replace_spaces_by
> >
> > But this is not a strong requirement, if you do not like the above
> > names and can't come up with something better, we can stick to the
> > current name.
> >
> > 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
> >   ... <snip>
> >   76     while (*ptr != '\0') {
> >   77         if (isspace(*ptr)){
> >   78             new_name = talloc_asprintf_append(...
> >   79         } else{
> >                ^^^^^
> >                   Missing space after else
> >   80             new_name = talloc_asprintf_append(...
> >   81         }
> >
> > The patch seem to work fine. But functions like
> > replace_whitespaces and the reverse version can be
> > easily unit-tested. It would be great if you could
> > add unit tests for these two.
>
> Attached patches fix also comments from other mails in this thread.
>
> LS

Thanks for adding the unit tests!

The patches look good to me and passed my testing. So ACK for
the code part. I would like to have a native English speaker
to review the man page changes in src/man/sssd.conf.5.xml

Michal

[Lukas Slebodnik]

On (25/07/14 15:15), Michal Židek wrote:
> On 07/25/2014 01:42 PM, Lukas Slebodnik wrote:
> > On (24/07/14 17:53), Michal Židek wrote:
> > > On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > > > ehlo,
> > > >
> > > > This patch add possibility to replace whitespace in user and group names with
> > > > a specified string. With string "-", sssd will return the same result as
> > > > winbind enabled option "winbind normalize names"
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/1854
> > > >
> > > > patch is attached.
> > > >
> > > > LS
> > >
> > > Hi Lukas,
> > >
> > > src/config/SSSDConfig/__init__.py.in
> > > 76     'whitespaces_replacement_string': _('All white spaces will be replaced
> > > in group name with this string'),
> > >
> > > Not just group name, but also user name. IMO something
> > > like this would be better:
> > > "All white spaces in group or user names will be replaced with
> > > this string." Or something similar.
> > >
> > > Also please put changes in
> > > src/config/SSSDConfig/__init__.py.in and man page changes into a
> > > separate patch. We will than ping a native speaker (Stephen ?)
> > > to check the wording when the coding part is accepted.
> > >
> > > The option name could be at least a little shorter, maybe:
> > > replace_whitespace_str
> > > or
> > > replace_spaces_by
> > >
> > > But this is not a strong requirement, if you do not like the above
> > > names and can't come up with something better, we can stick to the
> > > current name.
> > >
> > > 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
> > >  ... <snip>
> > >  76     while (*ptr != '\0') {
> > >  77         if (isspace(*ptr)){
> > >  78             new_name = talloc_asprintf_append(...
> > >  79         } else{
> > >               ^^^^^
> > >                  Missing space after else
> > >  80             new_name = talloc_asprintf_append(...
> > >  81         }
> > >
> > > The patch seem to work fine. But functions like
> > > replace_whitespaces and the reverse version can be
> > > easily unit-tested. It would be great if you could
> > > add unit tests for these two.
> >
> > Attached patches fix also comments from other mails in this thread.
> >
> > LS
>
> Thanks for adding the unit tests!
>
> The patches look good to me and passed my testing. So ACK for
> the code part. I would like to have a native English speaker
> to review the man page changes in src/man/sssd.conf.5.xml
>
> Michal

Attached are patches with different text in man page.
Thank you Dan.

LS

[Michal Židek]

On 07/25/2014 04:58 PM, Lukas Slebodnik wrote:
> On (25/07/14 15:15), Michal Židek wrote:
> > On 07/25/2014 01:42 PM, Lukas Slebodnik wrote:
> > > On (24/07/14 17:53), Michal Židek wrote:
> > > > On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > > > > ehlo,
> > > > >
> > > > > This patch add possibility to replace whitespace in user and group names with
> > > > > a specified string. With string "-", sssd will return the same result as
> > > > > winbind enabled option "winbind normalize names"
> > > > >
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/1854
> > > > >
> > > > > patch is attached.
> > > > >
> > > > > LS
> > > >
> > > > Hi Lukas,
> > > >
> > > > src/config/SSSDConfig/__init__.py.in
> > > > 76     'whitespaces_replacement_string': _('All white spaces will be replaced
> > > > in group name with this string'),
> > > >
> > > > Not just group name, but also user name. IMO something
> > > > like this would be better:
> > > > "All white spaces in group or user names will be replaced with
> > > > this string." Or something similar.
> > > >
> > > > Also please put changes in
> > > > src/config/SSSDConfig/__init__.py.in and man page changes into a
> > > > separate patch. We will than ping a native speaker (Stephen ?)
> > > > to check the wording when the coding part is accepted.
> > > >
> > > > The option name could be at least a little shorter, maybe:
> > > > replace_whitespace_str
> > > > or
> > > > replace_spaces_by
> > > >
> > > > But this is not a strong requirement, if you do not like the above
> > > > names and can't come up with something better, we can stick to the
> > > > current name.
> > > >
> > > > 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
> > > >   ... <snip>
> > > >   76     while (*ptr != '\0') {
> > > >   77         if (isspace(*ptr)){
> > > >   78             new_name = talloc_asprintf_append(...
> > > >   79         } else{
> > > >                ^^^^^
> > > >                   Missing space after else
> > > >   80             new_name = talloc_asprintf_append(...
> > > >   81         }
> > > >
> > > > The patch seem to work fine. But functions like
> > > > replace_whitespaces and the reverse version can be
> > > > easily unit-tested. It would be great if you could
> > > > add unit tests for these two.
> > >
> > > Attached patches fix also comments from other mails in this thread.
> > >
> > > LS
> >
> > Thanks for adding the unit tests!
> >
> > The patches look good to me and passed my testing. So ACK for
> > the code part. I would like to have a native English speaker
> > to review the man page changes in src/man/sssd.conf.5.xml
> >
> > Michal
>
> Attached are patches with different text in man page.
> Thank you Dan.
>
> LS

Thank you Dan.

ACK.

Michal

[Jakub Hrozek]

On Fri, Jul 25, 2014 at 07:10:20PM +0200, Michal Židek wrote:
> Thank you Dan.
>
> ACK.
>
> Michal

* master:
    21bc143c2855638242e9dfe01ea66198b5883b8a
    022c6b90bb37851c0e8704c0e5388ebc113c6470
    462db32918a05097652f8232cd6c8d78a826e63c
* sssd-1-11:
    c2859eec9d110d0062dbc5733fa842fd99d1edeb
    50d24652b5321f4cc5709bc0877dde5da4e67f08
    31cd021079212a78ebeb6ec2f10d66acc160f89b

[Pavel Reichl]

Hello Lukas,

just some nitpicks. Please see inline. Thanks!

On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> ehlo,
>
> This patch add possibility to replace whitespace in user and group names with
> a specified string. With string "-", sssd will return the same result as
> winbind enabled option "winbind normalize names"
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/1854
>
> patch is attached.
>
> LS
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> src/confdb/confdb.h                  |   1 +
>   src/config/SSSDConfig/__init__.py.in |   1 +
>   src/config/etc/sssd.api.conf         |   1 +
>   src/man/sssd.conf.5.xml              |  18 ++++
>   src/responder/nss/nsssrv.c           |   5 +
>   src/responder/nss/nsssrv.h           |   1 +
>   src/responder/nss/nsssrv_cmd.c       | 192 ++++++++++++++++++++++++++++++++++-
>   7 files changed, 218 insertions(+), 1 deletion(-)
>
> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> index ba33ea5d7123c6e799f13529b60c5938ad20e411..acb139adf6bc1ca2919906fe577333970ccdbfdd 100644
> --- a/src/confdb/confdb.h
> +++ b/src/confdb/confdb.h
> @@ -99,6 +99,7 @@
>   #define CONFDB_MEMCACHE_TIMEOUT "memcache_timeout"
>   #define CONFDB_NSS_HOMEDIR_SUBSTRING "homedir_substring"
>   #define CONFDB_DEFAULT_HOMEDIR_SUBSTRING "/home"
> +#define CONFDB_NSS_WHITESPACES_REPLACEMENT_STRING "whitespaces_replacement_string"
>
> /* PAM */
>   #define CONFDB_PAM_CONF_ENTRY "config/pam"
> diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
> index 439378ff86d07311f67d51ed775e2d973cd93869..910cdb4b5f0a8cca6ea3328b99722022e2f7ff24 100644
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -73,6 +73,7 @@ option_strings = {
>       'shell_fallback' : _('If a shell stored in central directory is allowed but not available, use this fallback'),
>       'default_shell': _('Shell to use if the provider does not list one'),
>       'memcache_timeout': _('How long will be in-memory cache records valid'),
>
> 'whitespaces_replacement_string': _('All white spaces will be replaced in group name with this string'),
>
> # [pam]
>  'offline_credentials_expiration' : _('How long to allow cached logins between online logins (days)'),
>
> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
> index 5e5a9284e9ad80d822fe1db4269353aeb7925682..2b4841cec3d5a52002a618a2f38d48a8948ef261 100644
> --- a/src/config/etc/sssd.api.conf
> +++ b/src/config/etc/sssd.api.conf
> @@ -44,6 +44,7 @@ shell_fallback = str, None, false
>   default_shell = str, None, false
>   get_domains_timeout = int, None, false
>   memcache_timeout = int, None, false
> +whitespaces_replacement_string = str, None, false
>
> [pam]
>   # Authentication service
> diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
> index 27d22f44ef4449ef9cad026757ff54dd5083672b..6f1c42b0aa3fb22e144fadc19880787e88fb860c 100644
> --- a/src/man/sssd.conf.5.xml
> +++ b/src/man/sssd.conf.5.xml
> @@ -645,6 +645,24 @@ fallback_homedir =/home/%u
>                           </para>
>                       </listitem>
>                   </varlistentry>
>            <varlistentry>
>                <term>whitespaces_replacement_string (string)</term>
>                <listitem>
>                    <para>
>                        This parameter controls whether sssd will replace
>                        whitespace in user and group names with a given
>                        string e.g. (_). For example, whether the name
>                        &quot;john doe&quot; should be replaced with
>                        the string &quot;john_doe&quot;. Frequently Unix
>                        shell scripts will have difficulty with usernames
>                        contains whitespace due to the default field
>                         separator in the shell.
>                    </para>
>                    <para>
>                        Default: not set (whitespaces will not be replaced)
>                    </para>
>                </listitem>
>            </varlistentry>
>         </variablelist>
>     </refsect2>
>     <refsect2 id='PAM'>
>
> diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
> index 84a6b7fedd096a7d4159b7ac6670820c1d8fd941..422dc6a05906eb2c2fbe5c32b3d0646d69b26891 100644
> --- a/src/responder/nss/nsssrv.c
> +++ b/src/responder/nss/nsssrv.c
> @@ -298,6 +298,11 @@ static int nss_get_config(struct nss_ctx *nctx,
>                               &nctx->homedir_substr);
>       if (ret != EOK) goto done;
> ret = confdb_get_string(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
>                        CONFDB_NSS_WHITESPACES_REPLACEMENT_STRING, NULL,
>                        &nctx->wsp_replacement_str);
> if (ret != EOK) goto done;
> ret = 0;
> done:
>     return ret;
>
> diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
> index a5b946b7e4a38d7d8b35ec5df1b6644d01896470..2e20b886ebbf2bc94ff7278b60b2f015f3eae74d 100644
> --- a/src/responder/nss/nsssrv.h
> +++ b/src/responder/nss/nsssrv.h
> @@ -69,6 +69,7 @@ struct nss_ctx {
>       char **etc_shells;
>       char *shell_fallback;
>       char *default_shell;
>
> char *wsp_replacement_str;
>
> struct sss_mc_ctx *pwd_mc_ctx;
>  struct sss_mc_ctx *grp_mc_ctx;
>
> diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> index a168a3e5d6116a1f6ca2c358707650c0e6c9e514..ec272ea06a92730e93ca811f5bc5e8e32380e22f 100644
> --- a/src/responder/nss/nsssrv_cmd.c
> +++ b/src/responder/nss/nsssrv_cmd.c
> @@ -32,6 +32,138 @@
>   #include "sss_client/idmap/sss_nss_idmap.h"
>   #include <time.h>
>
> +static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
>                                   const char *orig_name,
>                                   const char *replace_string){

missing space before {
> char *new_name;
> const char *ptr;
> size_t replace_string_len;
> TALLOC_CTX *tmp_ctx;
> if (replace_string == NULL || replace_string[0] == '\0') {
>    return orig_name;
> }
> replace_string_len = strlen(replace_string);
> /* faster implementations without multiple allocations */
> if (replace_string_len == 1) {
>    char *p;
>    new_name = talloc_strdup(mem_ctx, orig_name);
>    if (new_name == NULL) {
>        return NULL;
>    }
>    for (p = new_name; *p != '\0'; ++p) {
>        if (isspace(*p)){

missing space
>            *p = replace_string[0];
>        }
>    }
>    return new_name;
> }
> tmp_ctx = talloc_new(mem_ctx);
> if (tmp_ctx == NULL) {
>    return NULL;
> }
> new_name = talloc_strdup(tmp_ctx, "");
> if (new_name == NULL) {
>    goto done;
> }
> ptr = orig_name;
> while (*ptr != '\0') {
>    if (isspace(*ptr)){

missing space
>        new_name = talloc_asprintf_append(new_name, "%s", replace_string);
>    } else{
>        new_name = talloc_asprintf_append(new_name, "%c", *ptr);
>    }
>    if (new_name == NULL) {
>        goto done;;
>    }
>    ++ptr;
> }
> new_name = talloc_steal(mem_ctx, new_name);

please consider adding new line
> +done:
> talloc_free(tmp_ctx);
> return new_name;
>
> +}
>
> +static char *reverse_replace_withespaces(TALLOC_CTX *mem_ctx,
>                                     char *orig_name,
>                                     const char *replace_string){

missing space
> TALLOC_CTX *tmp_ctx;
> char *substr;
> char *new_name;
> const char *ptr = orig_name;
> size_t replace_string_len;
> if (replace_string == NULL || replace_string[0] == '\0') {
>    return orig_name;
> }
> replace_string_len = strlen(replace_string);
> /* faster implementations without multiple allocations */
> if (replace_string_len == 1) {
>    char *p;
>    new_name = talloc_strdup(mem_ctx, orig_name);
>    if (new_name == NULL) {
>        return NULL;
>    }
>    for (p = new_name; *p != '\0'; ++p) {
>        if (*p == replace_string[0] ){
>            *p = ' ';
>        }
>    }
>    return new_name;
> }
> tmp_ctx = talloc_new(mem_ctx);
> if (tmp_ctx == NULL) {
>    return NULL;
> }
> new_name = talloc_strdup(tmp_ctx, "");
> if (new_name == NULL) {
>    goto done;
> }
> do {
>    substr = strstr(ptr, replace_string);
>    if (substr != NULL) {
>        new_name = talloc_asprintf_append(new_name, "%.*s",
>                                          (int)(substr - ptr), ptr);
>        if (new_name == NULL) {
>            goto done;
>        }
>        ptr += substr - ptr;
>        ptr += replace_string_len;
>        /* append space */
>        new_name = talloc_asprintf_append(new_name, " ");
>        if (new_name == NULL) {
>            goto done;
>        }
>    } else {
>        new_name = talloc_asprintf_append(new_name, "%s", ptr);
>        if (new_name == NULL) {
>            goto done;
>        }
>        break;
>    }
> } while (substr != NULL);
> new_name = talloc_steal(mem_ctx, new_name);

You might consider adding new line here. If you don't want then don't.
> +done:
> talloc_free(tmp_ctx);
> return new_name;
>
> +}
> static int nss_cmd_send_error(struct nss_cmd_ctx *cmdctx, int err)
> {
>     return sss_cmd_send_error(cmdctx->cctx, err);
>
> @@ -371,6 +503,15 @@ static int fill_pwent(struct sss_packet *packet,
>                     "sss_get_cased_name failed, skipping\n");
>               continue;
>           }
>    tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
>                                 nctx->wsp_replacement_str);
>    if (tmpstr == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "replace_whitespaces failed, skipping\n");
>        continue;
>    }
>     to_sized_string(&name, tmpstr);
>
>     tmpstr = ldb_msg_find_attr_as_string(msg, SYSDB_GECOS, NULL);
>
> @@ -743,6 +884,14 @@ static int nss_cmd_getpwnam_search(struct nss_dom_ctx *dctx)
>           name = sss_get_cased_name(cmdctx, cmdctx->name, dom->case_sensitive);
>           if (!name) return ENOMEM;
>    name = reverse_replace_withespaces(dctx, name,
>                                       nctx->wsp_replacement_str);
>    if (name == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "reverse_replace_withespaces failed, skipping\n");
>        continue;
>    }
>     /* verify this user has not yet been negatively cached,
>     * or has been permanently filtered */
>     ret = sss_ncache_check_user(nctx->ncache, nctx->neg_timeout,
>
> @@ -2316,7 +2465,7 @@ static int fill_members(struct sss_packet *packet,
>       int memnum = *_memnum;
>       size_t rzero= *_rzero;
>       size_t rsize = *_rsize;
> char *tmpstr;
> const char *tmpstr;
>  struct sized_string name;
>  TALLOC_CTX *tmp_ctx = NULL;
>
> @@ -2344,6 +2493,14 @@ static int fill_members(struct sss_packet *packet,
>               continue;
>           }
>    tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
>                                 nctx->wsp_replacement_str);
>    if (tmpstr == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "replace_whitespaces failed, skipping\n");
>        continue;
>    }
>     if (nctx->filter_users_in_groups) {
>         ret = sss_ncache_check_user(nctx->ncache,
>                                     nctx->neg_timeout,
>
> @@ -2498,6 +2655,15 @@ static int fill_grent(struct sss_packet *packet,
>                     "sss_get_cased_name failed, skipping\n");
>               continue;
>           }
>    tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
>                                 nctx->wsp_replacement_str);
>    if (tmpstr == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "replace_whitespaces failed, skipping\n");
>        continue;
>    }
>     to_sized_string(&name, tmpstr);
>
>     /* fill in gid and name and set pointer for number of members */
>
> @@ -2692,6 +2858,14 @@ static int nss_cmd_getgrnam_search(struct nss_dom_ctx *dctx)
>           name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
>           if (!name) return ENOMEM;
>    name = reverse_replace_withespaces(dctx, name,
>                                       nctx->wsp_replacement_str);
>    if (name == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "reverse_replace_withespaces failed, skipping\n");
>        continue;
>    }
>     /* verify this group has not yet been negatively cached,
>     * or has been permanently filtered */

If you are changing nearby code can you add a space before *?
>       ret = sss_ncache_check_group(nctx->ncache, nctx->neg_timeout,
>
> @@ -3715,6 +3889,14 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
>           name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
>           if (!name) return ENOMEM;
>    name = reverse_replace_withespaces(dctx, name,
>                                       nctx->wsp_replacement_str);
>    if (name == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "reverse_replace_withespaces failed, skipping\n");
>        continue;
>    }
>     /* verify this user has not yet been negatively cached,
>     * or has been permanently filtered */

same as above please
>       ret = sss_ncache_check_user(nctx->ncache, nctx->neg_timeout,
>
> @@ -3874,6 +4056,14 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
>                   goto done;
>               }
>        name = reverse_replace_withespaces(dctx, name,
>                                           nctx->wsp_replacement_str);
>        if (name == NULL) {
>            DEBUG(SSSDBG_CRIT_FAILURE,
>                  "reverse_replace_withespaces failed, skipping\n");
>            continue;
>        }
>         /* For subdomains a fully qualified name is needed for
>          * sysdb_search_user_by_name and sysdb_search_group_by_name. */
>         if (IS_SUBDOMAIN(dom)) {

[Pavel Reichl]

Just a few more questions.

On 07/25/2014 08:40 AM, Pavel Reichl wrote:
> Hello Lukas,
>
> just some nitpicks. Please see inline. Thanks!
>
> On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
> > ehlo,
> >
> > This patch add possibility to replace whitespace in user and group names with
> > a specified string. With string "-", sssd will return the same result as
> > winbind enabled option "winbind normalize names"
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/1854
> >
> > patch is attached.
> >
> > LS
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > src/confdb/confdb.h                  |   1 +
> >   src/config/SSSDConfig/__init__.py.in |   1 +
> >   src/config/etc/sssd.api.conf         |   1 +
> >   src/man/sssd.conf.5.xml              |  18 ++++
> >   src/responder/nss/nsssrv.c           |   5 +
> >   src/responder/nss/nsssrv.h           |   1 +
> >   src/responder/nss/nsssrv_cmd.c       | 192 ++++++++++++++++++++++++++++++++++-
> >   7 files changed, 218 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > index ba33ea5d7123c6e799f13529b60c5938ad20e411..acb139adf6bc1ca2919906fe577333970ccdbfdd 100644
> > --- a/src/confdb/confdb.h
> > +++ b/src/confdb/confdb.h
> > @@ -99,6 +99,7 @@
> >   #define CONFDB_MEMCACHE_TIMEOUT "memcache_timeout"
> >   #define CONFDB_NSS_HOMEDIR_SUBSTRING "homedir_substring"
> >   #define CONFDB_DEFAULT_HOMEDIR_SUBSTRING "/home"
> > +#define CONFDB_NSS_WHITESPACES_REPLACEMENT_STRING "whitespaces_replacement_string"
> >
> > /* PAM */
> >   #define CONFDB_PAM_CONF_ENTRY "config/pam"
> > diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
> > index 439378ff86d07311f67d51ed775e2d973cd93869..910cdb4b5f0a8cca6ea3328b99722022e2f7ff24 100644
> > --- a/src/config/SSSDConfig/__init__.py.in
> > +++ b/src/config/SSSDConfig/__init__.py.in
> > @@ -73,6 +73,7 @@ option_strings = {
> >       'shell_fallback' : _('If a shell stored in central directory is allowed but not available, use this fallback'),
> >       'default_shell': _('Shell to use if the provider does not list one'),
> >       'memcache_timeout': _('How long will be in-memory cache records valid'),
> >
> > 'whitespaces_replacement_string': _('All white spaces will be replaced in group name with this string'),
> >
> > # [pam]
> >  'offline_credentials_expiration' : _('How long to allow cached logins between online logins (days)'),
> >
> > diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
> > index 5e5a9284e9ad80d822fe1db4269353aeb7925682..2b4841cec3d5a52002a618a2f38d48a8948ef261 100644
> > --- a/src/config/etc/sssd.api.conf
> > +++ b/src/config/etc/sssd.api.conf
> > @@ -44,6 +44,7 @@ shell_fallback = str, None, false
> >   default_shell = str, None, false
> >   get_domains_timeout = int, None, false
> >   memcache_timeout = int, None, false
> > +whitespaces_replacement_string = str, None, false
> >
> > [pam]
> >   # Authentication service
> > diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
> > index 27d22f44ef4449ef9cad026757ff54dd5083672b..6f1c42b0aa3fb22e144fadc19880787e88fb860c 100644
> > --- a/src/man/sssd.conf.5.xml
> > +++ b/src/man/sssd.conf.5.xml
> > @@ -645,6 +645,24 @@ fallback_homedir =/home/%u
> >                           </para>
> >                       </listitem>
> >                   </varlistentry>
> >            <varlistentry>
> >                <term>whitespaces_replacement_string (string)</term>
> >                <listitem>
> >                    <para>
> >                        This parameter controls whether sssd will replace
> >                        whitespace in user and group names with a given
> >                        string e.g. (_). For example, whether the name
> >                        &quot;john doe&quot; should be replaced with
> >                        the string &quot;john_doe&quot;. Frequently Unix
> >                        shell scripts will have difficulty with usernames
> >                        contains whitespace due to the default field
> >                         separator in the shell.
> >                    </para>
> >                    <para>
> >                        Default: not set (whitespaces will not be replaced)
> >                    </para>
> >                </listitem>
> >            </varlistentry>
> >         </variablelist>
> >     </refsect2>
> >     <refsect2 id='PAM'>
> >
> > diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
> > index 84a6b7fedd096a7d4159b7ac6670820c1d8fd941..422dc6a05906eb2c2fbe5c32b3d0646d69b26891 100644
> > --- a/src/responder/nss/nsssrv.c
> > +++ b/src/responder/nss/nsssrv.c
> > @@ -298,6 +298,11 @@ static int nss_get_config(struct nss_ctx *nctx,
> >                               &nctx->homedir_substr);
> >       if (ret != EOK) goto done;
> > ret = confdb_get_string(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
> >                        CONFDB_NSS_WHITESPACES_REPLACEMENT_STRING, NULL,
> >                        &nctx->wsp_replacement_str);
> > if (ret != EOK) goto done;
> > ret = 0;
> > done:
> >     return ret;
> >
> > diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
> > index a5b946b7e4a38d7d8b35ec5df1b6644d01896470..2e20b886ebbf2bc94ff7278b60b2f015f3eae74d 100644
> > --- a/src/responder/nss/nsssrv.h
> > +++ b/src/responder/nss/nsssrv.h
> > @@ -69,6 +69,7 @@ struct nss_ctx {
> >       char **etc_shells;
> >       char *shell_fallback;
> >       char *default_shell;
> >
> > char *wsp_replacement_str;
> >
> > struct sss_mc_ctx *pwd_mc_ctx;
> >  struct sss_mc_ctx *grp_mc_ctx;
> >
> > diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> > index a168a3e5d6116a1f6ca2c358707650c0e6c9e514..ec272ea06a92730e93ca811f5bc5e8e32380e22f 100644
> > --- a/src/responder/nss/nsssrv_cmd.c
> > +++ b/src/responder/nss/nsssrv_cmd.c
> > @@ -32,6 +32,138 @@
> >   #include "sss_client/idmap/sss_nss_idmap.h"
> >   #include <time.h>
> >
> > +static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
> >                                   const char *orig_name,
> >                                   const char *replace_string){
>
> missing space before {
> > char *new_name;
> > const char *ptr;
> > size_t replace_string_len;
> > TALLOC_CTX *tmp_ctx;
> > if (replace_string == NULL || replace_string[0] == '\0') {
> >    return orig_name;
> > }
> > replace_string_len = strlen(replace_string);
> > /* faster implementations without multiple allocations */
> > if (replace_string_len == 1) {
> >    char *p;
> >    new_name = talloc_strdup(mem_ctx, orig_name);
> >    if (new_name == NULL) {
> >        return NULL;
> >    }
> >    for (p = new_name; *p != '\0'; ++p) {
> >        if (isspace(*p)){
>
> missing space
> >            *p = replace_string[0];
> >        }
> >    }
> >    return new_name;
> > }
> > tmp_ctx = talloc_new(mem_ctx);

Is there any difference in creation of  tmp_ctx on NULL and on mem_ctx 
int this case? I guess as long as you steal result from tmp_ctx and then 
free tmp_ctx  it doesn't really matter, right? I guess it's just more 
usual to allocate on NULL in utility function.
> > if (tmp_ctx == NULL) {
> >    return NULL;
> > }
> > new_name = talloc_strdup(tmp_ctx, "");
> > if (new_name == NULL) {
> >    goto done;
> > }
> > ptr = orig_name;
> > while (*ptr != '\0') {
> >    if (isspace(*ptr)){
>
> missing space
> >        new_name = talloc_asprintf_append(new_name, "%s", replace_string);
> >    } else{
> >        new_name = talloc_asprintf_append(new_name, "%c", *ptr);
> >    }
> >    if (new_name == NULL) {
> >        goto done;;
> >    }
> >    ++ptr;
> > }
> > new_name = talloc_steal(mem_ctx, new_name);
>
> please consider adding new line
> > +done:
> > talloc_free(tmp_ctx);
> > return new_name;
> >
> > +}
> >
> > +static char *reverse_replace_withespaces(TALLOC_CTX *mem_ctx,
> >                                     char *orig_name,
> >                                     const char *replace_string){
>
> missing space
> > TALLOC_CTX *tmp_ctx;
> > char *substr;
> > char *new_name;
> > const char *ptr = orig_name;
> > size_t replace_string_len;
> > if (replace_string == NULL || replace_string[0] == '\0') {
> >    return orig_name;
> > }
> > replace_string_len = strlen(replace_string);
> > /* faster implementations without multiple allocations */
> > if (replace_string_len == 1) {
> >    char *p;
> >    new_name = talloc_strdup(mem_ctx, orig_name);
> >    if (new_name == NULL) {
> >        return NULL;
> >    }
> >    for (p = new_name; *p != '\0'; ++p) {
> >        if (*p == replace_string[0] ){
> >            *p = ' ';
> >        }
> >    }
> >    return new_name;
> > }
> > tmp_ctx = talloc_new(mem_ctx);
> > if (tmp_ctx == NULL) {
> >    return NULL;
> > }
> > new_name = talloc_strdup(tmp_ctx, "");
> > if (new_name == NULL) {
> >    goto done;
> > }
> > do {
> >    substr = strstr(ptr, replace_string);
> >    if (substr != NULL) {
> >        new_name = talloc_asprintf_append(new_name, "%.*s",
> >                                          (int)(substr - ptr), ptr);
> >        if (new_name == NULL) {
> >            goto done;
> >        }
> >        ptr += substr - ptr;
> >        ptr += replace_string_len;
> >        /* append space */
> >        new_name = talloc_asprintf_append(new_name, " ");

You were interested in saving allocations - could adding space be part 
of prev talloc_asprintf_append?
> >        if (new_name == NULL) {
> >            goto done;
> >        }
> >    } else {
> >        new_name = talloc_asprintf_append(new_name, "%s", ptr);
> >        if (new_name == NULL) {
> >            goto done;
> >        }
> >        break;

Why the break here? do-while loop would end anyway, right?
> >    }
> > } while (substr != NULL);
> > new_name = talloc_steal(mem_ctx, new_name);
>
> You might consider adding new line here. If you don't want then don't.
> > +done:
> > talloc_free(tmp_ctx);
> > return new_name;
> >
> > +}
> > static int nss_cmd_send_error(struct nss_cmd_ctx *cmdctx, int err)
> > {
> >     return sss_cmd_send_error(cmdctx->cctx, err);
> >
> > @@ -371,6 +503,15 @@ static int fill_pwent(struct sss_packet *packet,
> >                     "sss_get_cased_name failed, skipping\n");
> >               continue;
> >           }
> >    tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
> >                                 nctx->wsp_replacement_str);
> >    if (tmpstr == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "replace_whitespaces failed, skipping\n");
> >        continue;
> >    }
> >     to_sized_string(&name, tmpstr);
> >
> >     tmpstr = ldb_msg_find_attr_as_string(msg, SYSDB_GECOS, NULL);
> >
> > @@ -743,6 +884,14 @@ static int nss_cmd_getpwnam_search(struct nss_dom_ctx *dctx)
> >           name = sss_get_cased_name(cmdctx, cmdctx->name, dom->case_sensitive);
> >           if (!name) return ENOMEM;
> >    name = reverse_replace_withespaces(dctx, name,
> >                                       nctx->wsp_replacement_str);
> >    if (name == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "reverse_replace_withespaces failed, skipping\n");
> >        continue;
> >    }
> >     /* verify this user has not yet been negatively cached,
> >     * or has been permanently filtered */
> >     ret = sss_ncache_check_user(nctx->ncache, nctx->neg_timeout,
> >
> > @@ -2316,7 +2465,7 @@ static int fill_members(struct sss_packet *packet,
> >       int memnum = *_memnum;
> >       size_t rzero= *_rzero;
> >       size_t rsize = *_rsize;
> > char *tmpstr;
> > const char *tmpstr;
> >  struct sized_string name;
> >  TALLOC_CTX *tmp_ctx = NULL;
> >
> > @@ -2344,6 +2493,14 @@ static int fill_members(struct sss_packet *packet,
> >               continue;
> >           }
> >    tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
> >                                 nctx->wsp_replacement_str);
> >    if (tmpstr == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "replace_whitespaces failed, skipping\n");
> >        continue;
> >    }
> >     if (nctx->filter_users_in_groups) {
> >         ret = sss_ncache_check_user(nctx->ncache,
> >                                     nctx->neg_timeout,
> >
> > @@ -2498,6 +2655,15 @@ static int fill_grent(struct sss_packet *packet,
> >                     "sss_get_cased_name failed, skipping\n");
> >               continue;
> >           }
> >    tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
> >                                 nctx->wsp_replacement_str);
> >    if (tmpstr == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "replace_whitespaces failed, skipping\n");
> >        continue;
> >    }
> >     to_sized_string(&name, tmpstr);
> >
> >     /* fill in gid and name and set pointer for number of members */
> >
> > @@ -2692,6 +2858,14 @@ static int nss_cmd_getgrnam_search(struct nss_dom_ctx *dctx)
> >           name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
> >           if (!name) return ENOMEM;
> >    name = reverse_replace_withespaces(dctx, name,
> >                                       nctx->wsp_replacement_str);
> >    if (name == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "reverse_replace_withespaces failed, skipping\n");
> >        continue;
> >    }
> >     /* verify this group has not yet been negatively cached,
> >     * or has been permanently filtered */
>
> If you are changing nearby code can you add a space before *?
> >       ret = sss_ncache_check_group(nctx->ncache, nctx->neg_timeout,
> >
> > @@ -3715,6 +3889,14 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
> >           name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
> >           if (!name) return ENOMEM;
> >    name = reverse_replace_withespaces(dctx, name,
> >                                       nctx->wsp_replacement_str);
> >    if (name == NULL) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "reverse_replace_withespaces failed, skipping\n");
> >        continue;
> >    }
> >     /* verify this user has not yet been negatively cached,
> >     * or has been permanently filtered */
>
> same as above please
> >       ret = sss_ncache_check_user(nctx->ncache, nctx->neg_timeout,
> >
> > @@ -3874,6 +4056,14 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
> >                   goto done;
> >               }
> >        name = reverse_replace_withespaces(dctx, name,
> >                                           nctx->wsp_replacement_str);
> >        if (name == NULL) {
> >            DEBUG(SSSDBG_CRIT_FAILURE,
> >                  "reverse_replace_withespaces failed, skipping\n");
> >            continue;
> >        }
> >         /* For subdomains a fully qualified name is needed for
> >          * sysdb_search_user_by_name and sysdb_search_group_by_name. */
> >         if (IS_SUBDOMAIN(dom)) {
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, Jul 25, 2014 at 10:51:27AM +0200, Pavel Reichl wrote:
> > > tmp_ctx = talloc_new(mem_ctx);
>
> Is there any difference in creation of  tmp_ctx on NULL and on mem_ctx int
> this case? I guess as long as you steal result from tmp_ctx and then free
> tmp_ctx  it doesn't really matter, right? I guess it's just more usual to
> allocate on NULL in utility function.

We keep having discussions about this topic periodically :)

I personally prefer allocating on NULL for purely utility functions
where you have no idea who the caller is, purely because it's easy to
detect leaks with valgrind.

However, if the utility function is used as part of a request that is
guaranteed to end, I think it's OK (and actually should be preferred)
to allocate tmp_ctx on some existing context (like state or req in tevent
requests). That way, even if you forget to free tmp_ctx, the memory will
be gone when the request finishes.