D29810: Don't use the setenv function after fork

[Ben Cooksley]

bcooksley added a comment.


  NOTICE OF INTENTION TO REVERT
  
  Due to this commit introducing a FBTFS on FreeBSD, this commit will be 
automatically reverted in 24 hours unless 
https://invent.kde.org/frameworks/kcrash/-/merge_requests/3 has been merged and 
the FTBFS condition corrected.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: bcooksley, ahmadsamir, bruns, apol, anthonyfieroni, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29810: Don't use the setenv function after fork

[Ahmad Samir]

ahmadsamir added a comment.


  In D29810#674824 , @jpalecek wrote:
  
  > In D29810#674815 , @dfaure wrote:
  >
  > > This breaks FreeBSD compilation. Please check: 
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20FreeBSDQt5.14/17/
  >
  >
  > I see. It needs the declaration of `environ`, which is only provided on 
GNU. Should I amend it here?
  
  
  This has been committed already, create a new review request. (Note that KDE 
has moved to Gitlab at https://invent.kde.org, best if you create a merge 
request there; Phabricator still works, but it's planned to become read-only). 
:)

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: ahmadsamir, bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek added a comment.


  In D29810#674815 , @dfaure wrote:
  
  > This breaks FreeBSD compilation. Please check: 
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20FreeBSDQt5.14/17/
  
  
  I see. It needs the declaration of `environ`, which is only provided on GNU. 
Should I amend it here?

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[David Faure]

dfaure added a comment.


  This breaks FreeBSD compilation. Please check: 
https://build.kde.org/job/Frameworks/job/kcrash/job/kf5-qt5%20FreeBSDQt5.14/17/

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[David Faure]

dfaure closed this revision.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R285 KCrash

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek added a reviewer: dfaure.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks, dfaure
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek added inline comments.

INLINE COMMENTS

> bruns wrote in kcrash.cpp:698
> `std::copy_if`, instead of a separate `remove_if` pass.

Thanks for that suggestion, updated accordingly. I admit I'm still more 
accustomed to the old ways of `remove_if`.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek updated this revision to Diff 83160.
jpalecek marked an inline comment as done.
jpalecek added a comment.


  Change remove_if and copy to copy_if, per suggestion

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29810?vs=83137=83160

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29810

AFFECTED FILES
  src/kcrash.cpp

To: jpalecek, #frameworks
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> kcrash.cpp:689
>  {
> +static char autorestarted_envvar[] = "KCRASH_AUTO_RESTARTED";
> +char** environ_end;

move this to the lambda scope, you can also make the var name shorter then.

> kcrash.cpp:698
> +}
> +auto end = std::copy(environ, environ_end, environ_data.begin());
> +

`std::copy_if`, instead of a separate `remove_if` pass.

> kcrash.cpp:703
> + return strncmp(autorestarted_envvar, s, 
> sizeof(autorestarted_envvar)-1) == 0 &&
> + s[sizeof(autorestarted_envvar)-1] == '=';
> + });

the part after the `&&` can be removed when you include the '=' in the static 
string

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks
Cc: bruns, apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek marked an inline comment as done.
jpalecek added inline comments.

INLINE COMMENTS

> apol wrote in kcrash.cpp:823
> Use {} instead of ; for readability.

Done that.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks
Cc: apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek added a dependent revision: D29814: Fix segfault on no restart args.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29810

To: jpalecek, #frameworks
Cc: apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D29810: Don't use the setenv function after fork

[Jiří Paleček]

jpalecek updated this revision to Diff 83137.
jpalecek retitled this revision from "Don't use setenv after fork" to "Don't 
use the setenv function after fork".
jpalecek edited the summary of this revision.
jpalecek added a comment.


  Change the code a bit for readability

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29810?vs=83023=83137

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29810

AFFECTED FILES
  src/kcrash.cpp

To: jpalecek, #frameworks
Cc: apol, anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns