Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What about being more verbose than just "Error"? "Failed to execute script" or something like:
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.
I am not against removing the "Error", but cgi_error() function will also output "Cannot execute script ....", then it is kind of redundant to print "Failed to execute", but printing only the strerror(errno) without "Error" makes it a little confusing. Any idea on this?
@Lekensteyn
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.
I suggest to add a parameter to
cgi_error
that can be used to append additional information that should only be logged to the error log (instead of stdout). This can then be NULL for everything except the internal server error. What do you think of that?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.
I would prefer to keep consistent to the current definition of
cgi_error
, because adding additional argument will make all the cgi_error calling filled with a NULL expect the "Cannot execute script" one. Create log functionality for only one possible error is not useful too. What I am thinking is just make a string containing thestrerror
message and pass it intofilename
argument. Actually I should say, cgi_error is not a very nice design, although it looks simple, but it assume every variable string should append in the end, which is not the case.