Updated logs to match Apache HTTP Server Common Log Format style#22
Merged
Updated logs to match Apache HTTP Server Common Log Format style#22
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds two header-parsing macros, relocates timestamp buffers, introduces an Apache Combined Log Format Extended logger, parses User-Agent/Referer at shutdown and emits CLFE logs; README updated to advertise CLF support and that file logging is not yet supported. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Parser as "PARSE_UA/REFERER"
participant Time as "clock_gettime/localtime_r/strftime"
participant Logger as "LOG_REQUEST_CLFE"
Client->>Server: send HTTP request (headers + request line)
Server->>Parser: scan request buffer for Referer/User-Agent
Parser-->>Server: populate `referer`, `user_agent` buffers
Server->>Time: request localized timestamp
Time-->>Server: return formatted timestamp
Server->>Logger: supply IP, user, time, request, status, size, referer, user-agent
Logger->>Server: emit single CLFE log line to stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@macros/httputils.asm`:
- Around line 357-437: PARSE_UA_HEADER scans the entire buffer and only matches
exact "User-Agent" casing, so fix it to stop scanning at the end-of-headers
marker and perform a case-insensitive field-name compare: in the PARSE_UA_HEADER
macro (and the analogous Referer macro), change the scan loop to break when you
detect the header terminator sequence CR LF CR LF (0x0d0a0d0a) so you don't
march into the body, and replace the exact dword/byte comparisons for
"User-Agent: " with a case-folded comparison (e.g., normalize bytes to lowercase
by masking ASCII alphabetic characters or compare each letter allowing both
cases) when matching the field name and the following ": " delimiter; ensure the
copy logic and bounds checks (r8/r9/%2/%4) remain intact and set out_buf[0]=0
when not found.
In `@macros/logutils.asm`:
- Around line 370-392: The request-printing logic (labels %%pt5, %%req_scan,
%%req_print) currently emits the request buffer and quoted markers
(clfe_qm/clfe_qm_len and PRINT r10,r9) verbatim, allowing quotes and control
bytes (and bare LF 0x0A) to break logs; modify the scan and print path to (1)
treat 0x0A as a terminator in %%req_scan, (2) on print escape any double-quote
characters and non-printable/control bytes (e.g. emit \" for " and \xHH for
control bytes) instead of raw bytes, and (3) reuse the same sanitization for
other quoted fields (Referer and User-Agent) referenced in the later block
(lines 423-462) so all PRINT usages output safe, escaped strings rather than raw
input.
- Around line 407-422: The size column currently prints the prepared header
value content_length_b at label %%pt7/%%no_len, which reports the Content-Length
header rather than actual bytes sent; change both branches to compute and PRINT
the runtime body-bytes counter (the variable that tracks bytes actually written,
e.g., bytes_sent or response_body_bytes) instead of STRLEN
content_length_b/PRINT content_length_b. Locate the output code at labels %%pt7
and %%no_len and replace the STRLEN/PRINT of content_length_b with logic that
converts the actual bytes-written counter into rcx (string length) and then
PRINT that buffer, ensuring both successful and no-length branches use the real
bytes-sent value.
- Around line 324-339: The code in label %%pt3 prints the decoded username
(username / PRINT r10, rcx) based solely on strlen check, which can leak
caller-supplied names for failed auth; instead, change the branch to check the
actual authentication result flag set by program.asm (e.g., auth_ok or similar)
and only PRINT r10, rcx when that auth flag indicates success; otherwise jump to
%%no_auth and PRINT clfe_missing (the "-" token); ensure log_space/
log_space_len printing remains unchanged and that the new branch replaces the
current cmp rcx,0 / je %%no_auth logic so username is emitted only on successful
auth.
- Around line 144-149: The reserved size for status_buf is too small for the
ITOA macro which uses buffer + 19 as scratch space and will write past the end;
increase the reservation for status_buf (the resb value) to at least 20 bytes
(or to the exact required length used by ITOA) so the ITOA macro’s temporary
writes and subsequent copy-back into status_buf cannot overflow it.
In `@README.md`:
- Line 25: The README currently labels the access log as "Common Log Format" but
the new log line includes Referer and User-Agent, so update the documentation
text to call it "combined-format logging" (or "Apache combined log format")
instead of "Common Log Format" and mention that the access log includes the
extra Referer and User-Agent fields so consumers should not expect the 7-field
CLF; update the single line describing the logging to reflect "combined" and
note the additional fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47f29f84-bab7-4511-8cab-a129f4210166
📒 Files selected for processing (4)
README.mdmacros/httputils.asmmacros/logutils.asmprogram.asm
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
macros/logutils.asm (1)
222-223: Consider removing deprecated macro in a follow-up PR.The "Not used anymore!!" comment indicates this macro is obsolete. If it's truly unused, removing dead code improves maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@macros/logutils.asm` around lines 222 - 223, Remove the deprecated macro located next to the "Not used anymore!!" comment in macros/logutils.asm (the macro definition immediately adjacent to that comment); delete its entire definition and any internal references, search the repo for any remaining usages and update or remove those call sites, then run the assembler/build and tests to ensure nothing depends on it before committing as a follow-up PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@macros/logutils.asm`:
- Around line 452-464: The CLF output currently emits an extra trailing space by
printing log_space before the final LF in both the %%pt9 and %%no_ua branches;
update those branches (the PRINT calls for log_space, log_space_len) to stop
emitting the trailing separator—either remove the PRINT log_space call or reduce
its length so no space is output—and leave the final LF at label %%done
untouched; make this change for the PRINT invocations alongside PRINT
user_agent/PRINT clfe_qm so both branches produce no trailing space.
---
Nitpick comments:
In `@macros/logutils.asm`:
- Around line 222-223: Remove the deprecated macro located next to the "Not used
anymore!!" comment in macros/logutils.asm (the macro definition immediately
adjacent to that comment); delete its entire definition and any internal
references, search the repo for any remaining usages and update or remove those
call sites, then run the assembler/build and tests to ensure nothing depends on
it before committing as a follow-up PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e702e14c-d6e7-404b-8e9a-04024bb1cdfb
📒 Files selected for processing (2)
README.mdmacros/logutils.asm
✅ Files skipped from review due to trivial changes (1)
- README.md
|
|
Owner
Author
|
I should fix the workflow to run only once per pr ngl |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation