-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix fpm log prefix message with wrong stdout/stderr notation #4476
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
@@ -144,12 +144,24 @@ static void fpm_stdio_child_said(struct fpm_event_s *ev, short which, void *arg) | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be deleted
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
cbc2ee4
to
33074ba
Compare
@bukka Thank you for your reply. |
Merged as ffcf57f |
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