On (02/06/14 10:01), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
> On (01/06/14 19:23), Jakub Hrozek wrote:
> >On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
> >> On (27/05/14 16:32), Jakub Hrozek wrote:
> >> >On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
> >> >> On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
> >> >> > O
> >> >> > >The fact of passing pointer to the same area in memory to
2 separate
> >> >> > >arguments of sss_parse_name() is what I called potential
source of bugs.
> >> >> > >You said "It seems strange to me" so I hope you
know what I mean.
> >> >> > It is strange, but it isn't wrong.
> >> >> > * orig_name refers to old string
> >> >> > * homedir_ctx->username will refer to new string.
> >> >> > I need to use old string in debug message if function fails.
> >> >> I missed that.
> >> >>
> >> >> I did some testing and all seems to be working, so ACK to all
patches.
> >> >>
> >> >
> >> >In the third patch, you need to add the file
> >> >src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make
sure
> >> You ment homedir_substring.xml
> >
> >Yes :)
> >
> >>
> >> >it's processed for translations.
> >> >
> >> Added
> >>
> >> >Can you ask some native English speaker to check the contents of the
> >> >text added?
> >> >
> >> >As a side note, it would be nice to treat any refactoring as an
> >> >opportunity for adding more unit tests. Neither expand_homedir_template
> >> >nor sss_parse_name_const have any tests.
> >> The test for expand_homedir_template is in separate commit,
> >> because patches with refactoring are complicated enough.
> >>
> >> LS
> >
> >Thanks for the unit test!
> >
> >I don't have any other comments about functionality or code, just please
> >amend the man pages as Stephen suggested.
done
> will do after agreement about allocation of homedir_ctx.
> I do not want to send patchset more than once :-).
>
> >
> >One more improvement might be that you don't have to allocate the
> >homedir_ctx most of the time,
> It is just a *one* allocation and reason is to have a zero initialized
> structure. If you really want to avoid one call of talloc_zero I can replace it
> with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
done
This patch is targeted for sssd-1-11 and we're close to the
1.11.6
deadline. If you think you can spin up another version quickly, then
please do, otherwise let's clean up the unneeded allocation in 1.12.
new version attached
LS