fix fpm log prefix message with wrong stdout/stderr notation#4476
fix fpm log prefix message with wrong stdout/stderr notation#4476sadapon2008 wants to merge 1 commit intophp:PHP-7.3from
Conversation
|
@bukka, could you please review? |
sapi/fpm/fpm/fpm_stdio.c
Outdated
| zlog_stream_finish(log_stream); | ||
| } | ||
| // free stream's msg_prefix for reset | ||
| if (log_stream->msg_prefix) { |
There was a problem hiding this comment.
This would be better to handle inside zlog_stream_set_msg_prefix as it won't be necessary to do another allocation ("stdout" and "stderr" have the same length). However to make it generic there should be a check added. Basically something like:
zlog_bool zlog_stream_set_msg_prefix(struct zlog_stream *stream, const char *fmt, ...) /* {{{ */
{
char buf[MAX_WRAPPING_PREFIX_LENGTH];
size_t len;
va_list args;
if (!stream->decorate) {
return ZLOG_TRUE;
}
va_start(args, fmt);
len = vsnprintf(buf, MAX_WRAPPING_PREFIX_LENGTH - 1, fmt, args);
va_end(args);
if (stream->msg_prefix_len < len) {
stream->msg_prefix = stream->msg_prefix_len ? realloc(stream->msg_prefix, len + 1) : malloc(len + 1);
if (stream->msg_prefix == NULL) {
return ZLOG_FALSE;
}
}
memcpy(stream->msg_prefix, buf, len);
stream->msg_prefix[len] = 0;
stream->msg_prefix_len = len;
return len;
}
| zlog_stream_init_ex(log_stream, ZLOG_WARNING, STDERR_FILENO); | ||
| zlog_stream_set_decorating(log_stream, child->wp->config->decorate_workers_output); | ||
| zlog_stream_set_wrapping(log_stream, ZLOG_TRUE); | ||
| zlog_stream_set_msg_prefix(log_stream, "[pool %s] child %d said into %s: ", |
There was a problem hiding this comment.
This should be kept and then just added behind the stdout condition (line 152: https://github.com/php/php-src/pull/4476/files#diff-57f24d1c50be851c62c450ad57b13675R152 )
sapi/fpm/fpm/fpm_stdio.c
Outdated
| } | ||
| } | ||
| // reset stream's msg_prefix | ||
| zlog_stream_set_msg_prefix(log_stream, "[pool %s] child %d said into %s: ", |
sapi/fpm/fpm/fpm_stdio.c
Outdated
| zlog_stream_set_msg_prefix(log_stream, "[pool %s] child %d said into %s: ", | ||
| child->wp->config->name, (int) child->pid, is_stdout ? "stdout" : "stderr"); | ||
| // store fd type (stdout/stderr) | ||
| log_stream->is_stdout = (unsigned int)is_stdout; |
There was a problem hiding this comment.
It changes only if they are different. It should be also wrapped in inline function as other flags.
|
@bukka Thank you for your review. |
|
@sadapon2008 Sorry for the late review. The code looks good now. If you could squash it and push force it, that would be great! I will test it hopefully next weekend and if all good, I will merge it! Thanks! |
sadapon2008
force-pushed
the
fpm-log-prefix-message
branch
from
cbc2ee4 to
33074ba
Compare
|
@bukka Thank you for your reply. |
|
Merged as ffcf57f |
3 participants
This pr fixes https://bugs.php.net/bug.php?id=78334
The following code outputs messages to either stdout or stderr.
Expected results:
Actual results:
This is caused by the reuse of log stream introduced by #3471