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

Logging to syslog with dynamic error levels (related to #49467 and #66413) #1702

Closed
wants to merge 2 commits into from

Conversation

bestis
Copy link

@bestis bestis commented Dec 29, 2015

Hi,

There are couple of requests in bugs.php.net:
https://bugs.php.net/bug.php?id=66413 (Setting for default syslog facility)
https://bugs.php.net/bug.php?id=49467 (Errors are logged to syslog using LOG_NOTICE instead of LOG_ERR)

Both which has good idea, but why would we log all the errors with the same level as we have errors, warnings, etc.

This adds ability to have different log levels in syslog when error_log = syslog is set, so that different errors can be handled differently in syslogs and logging management services as graylog.

@weltling
Copy link
Contributor

weltling commented Apr 5, 2016

As for me this looks fine at first glance. Though i'd suggest additionally to keep the source compatibility for php_log_err() by introducing a macro instead of extending the arg list.

Thanks.

@bestis
Copy link
Author

bestis commented Apr 5, 2016

@weltling, could you give me example or point to one? I don't usually code C, I'm not so familiar with macros to make any changes by your suggestion.

@weltling
Copy link
Contributor

weltling commented Apr 5, 2016

Like following:

#define php_log_err(msg) php_log_err_with_serevity(msg, LOG_NOTICE)
PHPAPI ZEND_COLD void php_log_err_with_severity(char *log_message, int syslog_type_int);

So basically the function is renamed to something else and the exported symbol is not available anymore (so ABI breach), but that's no issue as the API number will be incremented anyway in 7.next. For that, the sources that used the old function name (say non core exts), will need no change and use the default error level. Just that some source level compat, if possible and makes sense, could be advantageous.

Thanks.

@bestis
Copy link
Author

bestis commented Apr 6, 2016

Ok, that seemed pretty straightforward so made that change and pushed it.

@weltling
Copy link
Contributor

weltling commented Jun 2, 2016

Merged with 3edf7d9, 0a04f61 and 73fd1fc.

Thanks!

@weltling weltling closed this Jun 2, 2016
@@ -1036,36 +1036,44 @@ static ZEND_COLD void php_error_cb(int type, const char *error_filename, const u
if (display && (EG(error_reporting) & type || (type & E_CORE))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a precedence case here with (EG(error_reporting) & type

@weltling
Copy link
Contributor

weltling commented Jun 2, 2016

@rmccullagh not sure what you mean. This is not something changed by this patch. Please explain.

Thanks.

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

Successfully merging this pull request may close these issues.

None yet

3 participants