[sssd PR#677][opened] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: opened

PR body:
"""
Some distributions want to drop pcre support. Sssd should work with
pcre2. With this patch sssd tries to use pcre2 if pcre is not present.

Resolves:
https://pagure.io/SSSD/sssd/issue/3833
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][comment] pcre: port to pcre2

[centos-ci]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

centos-ci commented:
"""
Can one of the admins verify this patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-429850535

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
add to whitelist
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-429936705

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
(Hopefully CI will run automatically for you the next time..)
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-429936817

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
CI detected some memory leaks on RHEL-7:
```
=11224== 228 bytes in 1 blocks are possibly lost in loss record 256 of 384
==11224==    at 0x4C29BC3: malloc (vg_replace_malloc.c:299)
==11224==    by 0x6F328F0: pcre_compile2 (in /usr/lib64/libpcre.so.1.2.0)
==11224==    by 0x54F56CB: sss_regexp_new (sss_regexp.c:90)
==11224==    by 0x54E90C4: sss_names_init_from_args (usertools.c:173)
==11224==    by 0x41713E: mock_domain (common_dom.c:205)
==11224==    by 0x41713E: create_multidom_test_ctx (common_dom.c:269)
==11224==    by 0x417225: create_dom_test_ctx (common_dom.c:304)
==11224==    by 0x410C2A: nested_groups_test_setup (test_nested_groups.c:607)
==11224==    by 0x410E72: nested_group_external_member_setup (test_nested_groups.c:826)
==11224==    by 0x4E3AA45: ??? (in /usr/lib64/libcmocka.so.0.5.0)
==11224==    by 0x4E3B312: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.5.0)
==11224==    by 0x404A8E: main (test_nested_groups.c:1330)
==11224== 
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: possible
   fun:malloc
   fun:pcre_compile2
   fun:sss_regexp_new
   fun:sss_names_init_from_args
   fun:mock_domain
   fun:create_multidom_test_ctx
   fun:create_dom_test_ctx
   fun:nested_groups_test_setup
   fun:nested_group_external_member_setup
   obj:/usr/lib64/libcmocka.so.0.5.0
   fun:_cmocka_run_group_tests
   fun:main
}
==11224== 228 bytes in 1 blocks are possibly lost in loss record 257 of 384
==11224==    at 0x4C29BC3: malloc (vg_replace_malloc.c:299)
==11224==    by 0x6F328F0: pcre_compile2 (in /usr/lib64/libpcre.so.1.2.0)
==11224==    by 0x54F56CB: sss_regexp_new (sss_regexp.c:90)
==11224==    by 0x54E90C4: sss_names_init_from_args (usertools.c:173)
==11224==    by 0x41713E: mock_domain (common_dom.c:205)
==11224==    by 0x41713E: create_multidom_test_ctx (common_dom.c:269)
==11224==    by 0x417225: create_dom_test_ctx (common_dom.c:304)
==11224==    by 0x410C2A: nested_groups_test_setup (test_nested_groups.c:607)
==11224==    by 0x4E3AA97: ??? (in /usr/lib64/libcmocka.so.0.5.0)
==11224==    by 0x4E3B1CD: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.5.0)
==11224==    by 0x404A8E: main (test_nested_groups.c:1330)
==11224== 
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: possible
   fun:malloc
   fun:pcre_compile2
   fun:sss_regexp_new
   fun:sss_names_init_from_args
   fun:mock_domain
   fun:create_multidom_test_ctx
   fun:create_dom_test_ctx
   fun:nested_groups_test_setup
   obj:/usr/lib64/libcmocka.so.0.5.0
   fun:_cmocka_run_group_tests
   fun:main
}
```

Can you check them, please?
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-429984629

[sssd PR#677][+Changes requested] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

Label: +Changes requested

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
I added some style comments. Please don't take them the wrong way, I didn't mean to needlessly pick on your code (I think in general the patches are good!) but because SSSD is such a big project already with a long development history, I just think it's quite important for all modules to look quite similar.
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-430355081

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][-Changes requested] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

Label: -Changes requested

[sssd PR#677][+Changes requested] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

Label: +Changes requested

[sssd PR#677][comment] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

thalman commented:
"""
Re-formatted to 80 columns
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-431803305

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][-Changes requested] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

Label: -Changes requested

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
retest this please
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-432579282

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
I'm sorry to keep beating the error code from *sss_regexp_new...but why not just return int from that function, which would be EOK on success, in which case a **self pointer would also be returned and if there is an error, just return an error code. The extended error message from pcre can be just printed with a debug message and then thrown away.

Currently it seems like the code tries too hard to emulate pcre exactly while also having the embedded self..

btw what strikes me as odd in particular is this:
```
159     ctx->illegal_path_re = sss_regexp_new(ctx, ILLEGAL_PATH_PATTERN, 0,               
160                                           &errval, &errstr, &errpos);                 
161     if (errval != 0) { 
```
...returning a pointer but not checing its value, but checking errval instead..
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-446365550

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][edited] WIP pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: WIP pcre: port to pcre2
Action: edited

Changed field: title
Original value:
"""
pcre: port to pcre2
"""

[sssd PR#677][comment] WIP pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: WIP pcre: port to pcre2

thalman commented:
"""
@jhrozek Please take a look at this update. Let me know if I get your suggestion right and I wil rebase it to one commit.
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-446977151

[sssd PR#677][synchronized] WIP pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: WIP pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][edited] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: edited

Changed field: title
Original value:
"""
WIP pcre: port to pcre2
"""

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
btw about testing: I cheated a little because removing pcre on Fedora removes also glib-devel. So I just removed one of the .pc files from pcre-devel and then configured sssd using --with-unicode-lib=libunistring.
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-507798111

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
One more question (and maybe I already asked it in the months before :-)) but did you consider preferring pcre2? Or would you prefer to do it in a separate patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-507798290

[sssd PR#677][comment] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

thalman commented:
"""
> One more question (and maybe I already asked it in the months before :-)) but did you consider preferring pcre2? Or would you prefer to do it in a separate patch?

Separate patch definitely, I have it ready (I needed it for testing)   
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-508024744

[sssd PR#677][synchronized] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][comment] pcre: port to pcre2

[thalman]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

thalman commented:
"""
I fixed formatting/space typos as well as errno_t definition issue.
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-508025769

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
Internal CI passed everywhere but Debian which is known to be broken -> Accepted.
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-508190215

[sssd PR#677][+Accepted] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

Label: +Accepted

[sssd PR#677][comment] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

jhrozek commented:
"""
* master: 2c965b04f693df4ca89eda7a4cff9d1900523837
"""

See the full comment at https://github.com/SSSD/sssd/pull/677#issuecomment-508238642

[sssd PR#677][closed] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Author: thalman
 Title: #677: pcre: port to pcre2
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/677/head:pr677
git checkout pr677

[sssd PR#677][+Pushed] pcre: port to pcre2

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/677
Title: #677: pcre: port to pcre2

Label: +Pushed