Conversation
…e, and grep filtering
…te related functionality
There was a problem hiding this comment.
Code Review
This pull request refactors the log subcommand to directly read log files from the CLI, bypassing the daemon socket. It introduces new features such as following logs in real-time (-f), specifying the number of lines to display (-n), and viewing daemon or all service logs. Feedback on these changes highlights a few critical issues: a race condition in the follow loop that can cause duplicate log output, the unintended omission of empty lines in the output, incorrect handling of -n 0 line counts, and a lack of validation for invalid -n arguments.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the logging system in ziginit to allow the CLI to directly read log files instead of querying the daemon via Unix sockets. This change enables a new follow mode (-f), custom line counts (-n), and direct viewing of daemon logs. The feedback highlights a potential silent data loss bug in followOutput for log lines exceeding 4KB, dangerous silent path truncations in path-building functions that should panic instead, and an inefficiency in followFiles where files are repeatedly opened and closed in the polling loop.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the log command in ziginit to directly read log files from the CLI, bypassing the daemon socket. It introduces new features such as follow mode (-f), custom line counts (-n), and unified logging (--all), along with comprehensive E2E tests. The review feedback highlights a potential data loss bug in the carry buffer handling of journal.zig when incoming data exceeds available space, an unsafe @ptrCast for sentinel-terminated pointers, and a compatibility issue in the Go tests due to the use of the min built-in introduced in Go 1.21.
| var avail = target.carry.len - target.carry_len; | ||
| if (avail == 0) { | ||
| if (prefix.len > 0) c.writeOnce(1, prefix); | ||
| c.writeOnce(1, target.carry[0..target.carry_len]); | ||
| target.carry_len = 0; | ||
| avail = target.carry.len; | ||
| } | ||
| const n = @min(data.len, avail); | ||
| @memcpy(target.carry[target.carry_len..][0..n], data[0..n]); | ||
| target.carry_len += n; | ||
| return; |
There was a problem hiding this comment.
There is a potential data loss bug here. If data.len > avail (where avail is the remaining space in target.carry), the code only copies avail bytes and silently discards the rest of data before returning. This directly violates the defensive programming guidelines in the custom style guide which states: "禁止静默丢弃数据:缓冲区满时必须有明确的降级策略(flush 后重试、报错退出、或截断警告),不能 @min(data.len, avail) 后默默丢弃超出部分".
To fix this, if data.len exceeds the available space, we should flush the current target.carry buffer to stdout first, reset target.carry_len to 0, and then copy the entire data chunk safely.
const avail = target.carry.len - target.carry_len;
if (data.len > avail) {
if (prefix.len > 0) c.writeOnce(1, prefix);
c.writeOnce(1, target.carry[0..target.carry_len]);
target.carry_len = 0;
}
@memcpy(target.carry[target.carry_len..][0..data.len], data);
target.carry_len += data.len;
return;
| pub fn pathZ(self: *const FollowTarget) [*:0]const u8 { | ||
| return @ptrCast(self.path_buf[0..self.path_len]); | ||
| } |
There was a problem hiding this comment.
Using @ptrCast on a non-sentinel-terminated slice to return a sentinel-terminated pointer ([*:0]const u8) is unsafe and non-idiomatic in Zig. Since self.path_buf is guaranteed to be null-terminated at self.path_len (as set in setPath), we can use Zig's sentinel-terminated slicing syntax self.path_buf[0..self.path_len :0].ptr to safely and cleanly obtain the pointer without casting.
pub fn pathZ(self: *const FollowTarget) [*:0]const u8 {
return self.path_buf[0..self.path_len :0].ptr;
}
| got := stdout.String() | ||
| if !strings.Contains(got, "FOLLOW:") || !strings.Contains(got, "OK") { | ||
| t.Fatalf("expected complete long line in follow output, got %d bytes: %s", len(got), got[:min(200, len(got))]) | ||
| } | ||
| if !strings.Contains(got, marker) { | ||
| t.Fatalf("long line was split or truncated: expected %d bytes marker, got: %s", len(marker), got[:min(200, len(got))]) | ||
| } |
There was a problem hiding this comment.
The built-in min function was introduced in Go 1.21. If this project is built or tested on older Go versions, using min will cause compilation errors. It is safer and more compatible to calculate the slice limit manually before performing the assertions.
got := stdout.String()
limit := 200
if len(got) < limit {
limit = len(got)
}
if !strings.Contains(got, "FOLLOW:") || !strings.Contains(got, "OK") {
t.Fatalf("expected complete long line in follow output, got %d bytes: %s", len(got), got[:limit])
}
if !strings.Contains(got, marker) {
t.Fatalf("long line was split or truncated: expected %d bytes marker, got: %s", len(marker), got[:limit])
}
No description provided.