Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pcntl get/set priority #9044

Closed

Conversation

juan-morales
Copy link
Contributor

@juan-morales juan-morales commented Jul 19, 2022

Code

Extension: pcntl
Functions: pcntl_setpriority() , pcntl_getpriority()

The problem

The PHP code to handle the errors for the mentioned functions is Linux aware only. Mac OS has a different behavior and uses the same error code for different things; plus FreeBSD does not respond in the same way as Linux or Mac under certain circumstances.

Because this feature (managing processes) is very OS dependent we cannot fully test the whole code, and also we cannot manage it in a uniform way (one code for all of them).

Documentation per OS related to possible error codes that can be return for the getpriority() and setpriority() functions

Errors per OS:
Mac
image

FreeBSD
image

Linux
image

The PHP code at the moment (master)

pcntl_getpriority()
image

pcntl_setpriority()
image

So as we can see the code to manage the errors is wrong as it will work as expected in Linux and FreeBSD, but not in MacOS.

This change pretends to fix this issue.

Relates to (already closed):
#8994
#8993

This is the first time I do such a change, please keep that in mind and advise me about anything that should be change, delete, adjust, etc.

Thanks in advance to all reviewers.

@cmb69 as promised ... here it is.

@juan-morales
Copy link
Contributor Author

(is really hard to test it before pushing when you don't have a Mac)

@juan-morales juan-morales changed the title [Work in progress] Fix pcntl get/set priority Fix pcntl get/set priority Jul 19, 2022
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I'll try to do a full review tomorrow, but left some comments for things that caught my attention while skimming over the patch.

@@ -1274,8 +1275,22 @@ PHP_FUNCTION(pcntl_getpriority)
php_error_docref(NULL, E_WARNING, "Error %d: No process was located using the given parameters", errno);
break;
case EINVAL:
#ifdef PRIO_DARWIN_BG
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preprocessor directives should always start at the beginning of a line (there may be whitespace between the # and the symbol, but in this case this would not be helpful:

Suggested change
#ifdef PRIO_DARWIN_BG
#ifdef PRIO_DARWIN_BG

Same for the other new directives in this file.

ext/pcntl/tests/pcntl_getpriority_error.phpt Outdated Show resolved Hide resolved
@juan-morales
Copy link
Contributor Author

juan-morales commented Jul 20, 2022

Thank you for the PR! I'll try to do a full review tomorrow, but left some comments for things that caught my attention while skimming over the patch.

Thanks @cmb69 a lot! I will wait for your full review before I do the suggested changes (so the pipeline does not have to run again)

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic makes sense to me. Just those small NITs from @cmb69 .

@juan-morales
Copy link
Contributor Author

The logic makes sense to me. Just those small NITs from @cmb69 .

Thanks for the review; and yes, I will apply the NITs from @cmb69 after the full review.

PD: I dont know what is better for you people, if

  1. Keep the code as it is so I can get a feedback as rich as possible, and work it out from there
    or
  2. I fix the NITs now and review it again later

I do whatever is better for the process

@bukka
Copy link
Member

bukka commented Jul 20, 2022

I just did the full review ;)

@bukka
Copy link
Member

bukka commented Jul 20, 2022

There's not that much to review though excepting checking the docs and logic.

@bukka
Copy link
Member

bukka commented Jul 20, 2022

Please fix those NITs, squash it to a single commit and push force it here.

@juan-morales juan-morales force-pushed the fix-pcntl_get/set-priority branch 2 times, most recently from 5ae27f7 to 01b698e Compare July 22, 2022 16:08
@juan-morales
Copy link
Contributor Author

@cmb69 @bukka change ready. But TRAVIS is failing ,according to the logs I saw is failing in a test that is not from my change. Can you advice on this?

@juan-morales juan-morales requested a review from bukka July 27, 2022 07:43
@cmb69 cmb69 closed this in 520bb2e Jul 27, 2022
@cmb69
Copy link
Member

cmb69 commented Jul 27, 2022

That Travis CI failure is intermittent, so not related to this PR.

Thank you for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants