Fix Parent property on processes with complex name #17545

jborean93 · 2022-06-20T21:20:45Z

PR Summary

Fix the Parent property to retrieve the parent process on Linux where
the process name has complex characters like a space or parenthesis.

PR Context

Reads from /proc/[pid]/status rather than /proc/[pid]/stat on Linux to get the parent process id. The status file is nicer as each entry is on a newline and has a label making it easier to select the data required. This is an alternative approach to deal with process' with a space and parenthesis in the name avoiding the last index of logic or trying to correctly select the proper column in the stat file.

Fixes #12908
Fixes #17541

Alternative to #12925

PR Checklist

iSazonov · 2022-06-21T03:54:42Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

-                        return invalidPid;
+                        Match ppidMatch = Regex.Match(
+                            line,
+                            @"^PPid:\s+(\d+)$",


The proposal looks very expensive. I'd prefer #12925.

Is the concern around the regex, the reading of multiple lines or something else? If it's the regex this could be changes to just doing line.StartsWith("PPid: ") and extract it from there but I thought regex gives some nicer validation.

My only concern about #12925 is if any extra entries are added to the stat file in the future that could mess with the LastIndexOf(')') check. I have no idea what the chances are of that happening but the file certainly has had additions in the past so I can't rule it out.

the additional cost of the regex is probably not worth it as the StartsWith("PPid: ") will do the trick. If you're really stuck on using regex, i'd be much happier if this was compiled once and continually reused.

@jborean93 is there any concern using StartsWith() and then splitting out the pid trimming the whitespace instead of a regex?

The regex just ensure it's PPid: digit but considering this is probably mandated in the spec it's probably not something to be concerned about. I can change it to StartsWith("PPid: ") but didn't think regex would be that bad of a performance problem and is somewhat simpler to extract the match.

@jborean93 Any chance to get the proposed changes done? Thanks!

Sorry will get to this next week, just on holidays at the moment.

Has been updated to use StartsWith instead as per the suggestion.

msftbot · 2022-06-28T14:01:17Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

daxian-dbw · 2022-06-28T17:27:16Z

@JamesWTruher can you please review this PR?

msftbot · 2022-07-06T02:01:20Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

Fix the `Parent` property to retrieve the parent process on Linux where the process name has complex characters like a space or parenthesis.

jborean93 · 2022-09-19T22:25:11Z

Looks like there might be a problem with using testdrive: in Pester. The permissions don't allow an unauthenticated users to set the executable bit breaking the tests for that scenario. Any suggestions on what to do there. I don't fully know how the tests are actually set up here.

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

pull-request-quantifier · 2022-09-20T19:58:09Z

This PR has 47 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +34 -13
Percentile : 18.8%

Total files changed: 2

Change summary by file extension:
.cs : +13 -6
.ps1 : +21 -7

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 👌 👎 (Email)
Customize PullRequestQuantifier for this repository.

jborean93 · 2022-09-20T20:30:01Z

Looks like there might be a problem with using testdrive: in Pester

Using /tmp as the path was enough to get it passing. Because it only runs on non-Windows that should be fine.

iSazonov

LGTM in common.

If we don't want extra allocations, we could replace Split() by ReadonlySpan.TrimStart()

iSazonov · 2022-09-21T03:48:10Z

test/powershell/Modules/Microsoft.PowerShell.Management/Get-Process.Tests.ps1

+while true; do sleep 1; done
+'@
+
+        # Can't use testdrive: as unelevated user doesn't have perms in test in CI


Please reword:

Suggested change

# Can't use testdrive: as unelevated user doesn't have perms in test in CI

# Can't use 'Testdrive:' as unelevated user doesn't have permissions to do 'chmod'.

I guess it is because .Net 7 RC1 has a breaking change in Directory.Create() where chmod mask was changed. If so, it might still break something in PowerShell or Pester.
/cc @nohwnd

I assumed this was just a byproduct of how PowerShell CI is set up. I've not looked into it but I believe the unelevated user may potentially be inheriting the same tmpdir as the elevated account and thus cannot set the execute bit on files in that dir. This is just a guess so your assumption could also be true.

daxian-dbw

LGTM. Thanks @jborean93!

msftbot · 2022-09-29T02:01:21Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov · 2022-09-29T03:26:15Z

@daxian-dbw Ready to merge?

jborean93 · 2022-09-29T04:48:54Z

Thanks everyone for your reviews and suggestions.

iSazonov · 2022-09-29T05:48:24Z

@jborean93 Thanks for your contribution!

jborean93 mentioned this pull request Jun 20, 2022

Parent Process Object is Missing #17541

Closed

pull-request-quantifier bot added the Extra Small label Jun 20, 2022

msftbot bot assigned daxian-dbw Jun 20, 2022

iSazonov reviewed Jun 21, 2022

View changes

msftbot bot added the Review - Needed The PR is being reviewed label Jun 28, 2022

daxian-dbw requested a review from JamesWTruher Jun 28, 2022

msftbot bot removed the Review - Needed The PR is being reviewed label Jun 28, 2022

msftbot bot added the Review - Needed The PR is being reviewed label Jul 6, 2022

iSazonov reviewed Aug 23, 2022

View changes

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Show resolved Hide resolved

daxian-dbw added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Sep 12, 2022

msftbot bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 12, 2022

jborean93 added 2 commits Sep 19, 2022

Fix Parent property on processes with complex name

2d0d3d4

Fix the `Parent` property to retrieve the parent process on Linux where the process name has complex characters like a space or parenthesis.

Use StartsWith instead of Regex to parse file

f8adcee

jborean93 force-pushed the linux-ppid branch from 8384983 to f8adcee Compare Sep 19, 2022

Remove now unused code

9d3c688

iSazonov reviewed Sep 20, 2022

View changes

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Show resolved Hide resolved

Update src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

a4d5472

iSazonov reviewed Sep 20, 2022

View changes

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Outdated Show resolved Hide resolved

iSazonov and others added 2 commits Sep 20, 2022

Update src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

ed8c6db

Try using just /tmp for test

025f30d

iSazonov approved these changes Sep 21, 2022

View changes

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 21, 2022

daxian-dbw approved these changes Sep 21, 2022

View changes

msftbot bot added the Review - Needed The PR is being reviewed label Sep 29, 2022

msftbot bot removed the Review - Needed The PR is being reviewed label Sep 29, 2022

daxian-dbw merged commit d66b2a5 into PowerShell:master Sep 29, 2022
39 checks passed

jborean93 deleted the linux-ppid branch Sep 29, 2022

TravisEz13 mentioned this pull request Sep 30, 2022

Move ApiScan to compliance build #18191

Merged

22 tasks

jborean93 mentioned this pull request Nov 18, 2022

Move Windows pinvokes #18603

Merged

22 tasks

Nov	DEC	Jan
	21
2021	2022	2023

Fix Parent property on processes with complex name #17545

Fix Parent property on processes with complex name #17545

jborean93 commented Jun 20, 2022

iSazonov Jun 21, 2022

jborean93 Jun 21, 2022

JamesWTruher Jul 18, 2022 •

edited

SteveL-MSFT Aug 22, 2022

jborean93 Aug 22, 2022 •

edited

daxian-dbw Sep 12, 2022

jborean93 Sep 12, 2022

jborean93 Sep 19, 2022

msftbot bot commented Jun 28, 2022

daxian-dbw commented Jun 28, 2022

msftbot bot commented Jul 6, 2022

jborean93 commented Sep 19, 2022

pull-request-quantifier bot commented Sep 20, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

jborean93 commented Sep 20, 2022

iSazonov left a comment

iSazonov Sep 21, 2022

jborean93 Sep 21, 2022

daxian-dbw left a comment

msftbot bot commented Sep 29, 2022

iSazonov commented Sep 29, 2022

jborean93 commented Sep 29, 2022

iSazonov commented Sep 29, 2022

	# Can't use testdrive: as unelevated user doesn't have perms in test in CI
	# Can't use 'Testdrive:' as unelevated user doesn't have permissions to do 'chmod'.

Fix Parent property on processes with complex name #17545

Fix Parent property on processes with complex name #17545

Conversation

jborean93 commented Jun 20, 2022

PR Summary

PR Context

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jborean93 Aug 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msftbot bot commented Jun 28, 2022

daxian-dbw commented Jun 28, 2022

msftbot bot commented Jul 6, 2022

jborean93 commented Sep 19, 2022

pull-request-quantifier bot commented Sep 20, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

jborean93 commented Sep 20, 2022

iSazonov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment

msftbot bot commented Sep 29, 2022

iSazonov commented Sep 29, 2022

jborean93 commented Sep 29, 2022

iSazonov commented Sep 29, 2022

JamesWTruher Jul 18, 2022 •

edited

jborean93 Aug 22, 2022 •

edited