On Mon, Jun 02, 2014 at 05:16:05PM +0200, Lukas Slebodnik wrote:
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
Perfect, thank you.
We can decide about ZERO_STRUCT/{ 0 } later, but I'd like to include
these patches in 1.11.6 and the deadline is looming..
The patches work fine and look good to me.
ACK