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

Log error cause when opcache cannot write to file cache #9258

Merged
merged 4 commits into from
Sep 3, 2022

Conversation

arnaud-lb
Copy link
Member

When using opcache.file_cache, opcache can log the following error when it fails to write a cache file:

Warning opcache cannot write to file '/cache/foo/bar.php'

It is not always obvious why it failed. This PR adds the cause of the failure, for example:

Warning opcache cannot write to file '/cache/foo/bar.php': No space left on device

ext/opcache/zend_file_cache.c Outdated Show resolved Hide resolved
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Implementation LGTM (the test doesn't seem to work though)

@arnaud-lb arnaud-lb marked this pull request as ready for review August 6, 2022 12:09
@arnaud-lb arnaud-lb requested a review from devnexen August 6, 2022 12:09
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LCTM

Comment on lines 13 to 14
posix_setrlimit(POSIX_RLIMIT_FSIZE, 1, 1) || die('skip Test requires setrlimit(RLIMIT_FSIZE) to work');
ini_parse_quantity(ini_get('opcache.jit_buffer_size')) === 0 || die('skip File cache is disabled when JIT is on');
Copy link
Member

Choose a reason for hiding this comment

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

I, personally, dislike this pattern (regardless of whether || or or is used), and would go with a single line if statement.

Copy link
Member

@iluuu1994 iluuu1994 Aug 6, 2022

Choose a reason for hiding this comment

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

Same. Although the style guide says you should add braces to if statements. (Although granted that's for C)

Copy link
Member

Choose a reason for hiding this comment

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

Then I'd rather change the style guide, than to find some loopholes. :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I don't find single line ifs to be an issue. IMO the only justified concern is:

#define FOO() foo(); bar()

if (baz) FOO();

Most of our macros guard against this with a do {} while(0); loop though.

@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.2 September 2, 2022 14:56
@arnaud-lb arnaud-lb merged commit ea1287b into php:PHP-8.2 Sep 3, 2022
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Sep 3, 2022
* PHP-8.2:
  [ci skip] NEWS
  Log the cause of error when opcache cannot write to file cache (php#9258)
  Fix high opcache.interned_strings_buffer causing shm corruption (php#9260)
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.

4 participants