Add %byte{value} logformat code for logging or sending arbitrary bytes #236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No support for zero byte values yet because existing Format::assemble() code does not support that out of the box, and there is no known _need_ for such support. It can be added later (without backward compatibility problems) if needed.
by reusing a new ParseInteger() template.
rousskov
requested changes
Nov 9, 2023
Also separated this function into Signed and Unsigned ones, to allow the caller be specific about the input it expects.
An "internal implementation detail" namespace is often called one of those three words[^1], abbreviated or not, with an underscore suffix or not. Let's standardize on "Detail_" because it avoids abbreviations, does not risk conflicting with "impl" commonly used in Pimpl idiom, and cannot conflict with any regular Squid namespace (because of the trailing underscore which, to many, also implies some kind of "private use"). [^1]: https://stackoverflow.com/a/26546780
... even though the existing callers always supply Integer types that include zero.
... because the vast majority of other values lack such descriptions, and there is a significant chance of getting these descriptions wrong unless we generate this code. FWIW, the mapping is already fairly easy to find by searching for the ByteCode name.
Unlike LFT_STRING, LFT_BYTE corresponds to a logformat code, so it should be together with other %codes.
.. because those changes * mistreated zero port (it should be rejected) * lost a valuable static_assert checking whether Port type can represent the maximum valid port number * added tok.reset() call that goes against Tokenizer parsing flow This code should be refactored to use Parser::UnsignedDecimalInteger(), but that refactoring should wait for the non-CONNECT port parsing to be upgraded to Tokenizer (and existing TODO) because that upgrade is likely to influence this refactoring.
... because that code needs to support parsing of octal and hex integers (in addition to decimal integers). I did not check for other problems. To convert this code, we need a more powerful/flexible parser.
Branch code used the wrong value type, allowing much larger values than PROXY protocol specs and, arguably, Squid code allow. Also documented a UnsignedDecimalInteger() bug that may affect how the design of branch code moves forward.
This commit effectively reverses branch commit 2f72f5f that attempted to reduce code duplication in parsing integers. Additional subsequent attempts were reverted previously. All these attempts were mostly correct. However, small-but-critical differences in calling code needs make reuse of a simple integer parser function virtually impossible. We will reduce this code duplication using a configurable parser. I even have a sketch we can start from, but that (and all the caller changes) deserve a dedicated project.
... because we use them in a static_assert(). Let's be explicit about our genuine needs rather than relying on optional[^1] compiler abilities to accommodate them. [^1]: https://stackoverflow.com/a/64501230
... to avoid missing them again. See also: Branch commit 29d34c9.
| @@ -4685,6 +4685,13 @@ DOC_START | |||
| Format codes: | |||
|
|
|||
| % a literal % character | |||
|
|
|||
| byte{value} Adds a single byte with the given value (e.g., %byte{10} | |||
| adds an ASCII LF character a.k.a. "new line" or "\n"). The value | |||
There was a problem hiding this comment.
Just to clarify: adding %byte{10} results in a genuine new line in access.log (i.e., not a couple of characters "\n"). Is this an intended/expected behavior?
There was a problem hiding this comment.
Is this an intended/expected behavior?
Yes, it is. In fact, IIRC, adding new lines is the use case that prompted the addition of this feature. Nearly all other ASCII characters can probably be added (as those characters) without a new logformat %code.
Adding new lines is useful when, for example, access records go to a logging daemon that expects HTTP header-like record syntax.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No support for zero byte values yet because existing Format::assemble()
code does not support that out of the box, and there is no known need
for such support. It can be added later (without backward compatibility
problems) if needed.