Print error summary#399
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds verbose interval rendering for CD and DVD dump error ranges, and introduces a debug command that scans state files and logs contiguous state runs. ChangesInterval range reporting
Debug state command
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dvd/dvd_dump.ixx (1)
1374-1386: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse-after-move:
error_intervalsis moved before it is read, so the dump "LBA error ranges" never print.At Line 1376
error_intervalsis moved intoctx.refine_cache, leaving it in a moved-from (empty) state. The subsequentif(!error_intervals.empty())at Line 1382 then evaluates on the moved-from object, so the guard is false and the LBA error-range block (the feature this PR adds) is silently skipped.error_intervals.to_string()would also be empty if reached.Print the ranges before transferring ownership into the cache.
🐛 Proposed fix (print before move)
if(dump) { - ctx.refine_cache = DvdRefineCache{ std::move(error_intervals), errors.scsi, errors.edc }; - LOG("media errors: "); LOG(" SCSI: {}", errors.scsi); LOG(" EDC: {}", errors.edc); if(!error_intervals.empty()) { LOG(""); LOG("LBA error ranges: {}", error_intervals.to_string()); } + + ctx.refine_cache = DvdRefineCache{ std::move(error_intervals), errors.scsi, errors.edc }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dvd/dvd_dump.ixx` around lines 1374 - 1386, The dump path in `dvd_dump.ixx` reads `error_intervals` after it has already been moved into `ctx.refine_cache`, so the LBA error-range log never runs. In the `dump` block, print the “LBA error ranges” message and call `error_intervals.to_string()` before constructing `DvdRefineCache` in `ctx.refine_cache`, or otherwise preserve the value before the move so the `if(!error_intervals.empty())` check and logging use the original object.
🧹 Nitpick comments (1)
interval_set.ixx (1)
218-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRange separator
-is ambiguous for negative values.The interval boundary logic is correct for the half-open
[start, end)convention (single value whenend - start == 1, elsestart-(end-1)). However, the-separator becomes hard to read for negativeTvalues — e.g. a CD pregap range{-150, -100}renders as-150--101. The DVD structure logging in this codebase already uses..(e.g.dvd/dvd_dump.ixxline 257PSN: [{} .. {}]), and the PR description mentions switching the separator to... Aligning here improves readability and consistency.♻️ Proposed change to use `..` separator
- str += end - start == 1 ? std::format("{}", start) : std::format("{}-{}", start, end - 1); + str += end - start == 1 ? std::format("{}", start) : std::format("{}..{}", start, end - 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interval_set.ixx` around lines 218 - 229, The `to_string()` formatter in `interval_set` uses `-` between range bounds, which becomes ambiguous for negative values and inconsistent with existing logging style. Update the range rendering in `to_string()` to use `..` for non-singleton intervals while keeping the half-open `[start, end)` logic intact, and leave the single-value case unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@debug.ixx`:
- Line 446: The STATE_NAME lookup in the state-handling code can read past the
5-element array when the raw byte from the .state file is unexpected. Update the
logic around STATE_NAME in the state parsing/printing path to validate the
uint8_t index before indexing, and fall back to a safe default or error path for
out-of-range values. Make sure the same bounds check is applied anywhere the raw
state byte is converted to a STATE_NAME entry, including the related uses near
the state decode/display code.
- Around line 460-461: The CD branch in debug.ixx is converting negative sample
positions to LBA with integer division in LOG, which truncates toward zero and
misreports lead-in values. Update the range display logic around the cd check so
the sample-to-LBA conversion floors negative values instead of relying on /
CD_DATA_SIZE_SAMPLES, using the existing range_start/range_end and
CD_DATA_SIZE_SAMPLES symbols to keep the visual state output accurate.
In `@dvd/dvd_dump.ixx`:
- Around line 1388-1398: The refine/non-dump logging in dvd_dump.ixx is checking
the wrong container: intervals has been drained by the interval-processing loop,
so the correction summary never reports sectors that actually remained
uncorrected. Update the logging in the refine path to use the failure-tracking
collection (error_intervals, or an equivalent dedicated uncorrected set) instead
of intervals, and make sure the logic near the correction statistics block
reports the attempted-but-still-bad LBA ranges.
---
Outside diff comments:
In `@dvd/dvd_dump.ixx`:
- Around line 1374-1386: The dump path in `dvd_dump.ixx` reads `error_intervals`
after it has already been moved into `ctx.refine_cache`, so the LBA error-range
log never runs. In the `dump` block, print the “LBA error ranges” message and
call `error_intervals.to_string()` before constructing `DvdRefineCache` in
`ctx.refine_cache`, or otherwise preserve the value before the move so the
`if(!error_intervals.empty())` check and logging use the original object.
---
Nitpick comments:
In `@interval_set.ixx`:
- Around line 218-229: The `to_string()` formatter in `interval_set` uses `-`
between range bounds, which becomes ambiguous for negative values and
inconsistent with existing logging style. Update the range rendering in
`to_string()` to use `..` for non-singleton intervals while keeping the
half-open `[start, end)` logic intact, and leave the single-value case
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6746c5f-0b92-4c44-a421-21d8b7ae8227
📒 Files selected for processing (5)
cd/cd_dump.ixxdebug.ixxdvd/dvd_dump.ixxinterval_set.ixxredumper.ixx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dvd/dvd_dump.ixx (1)
1382-1393: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRefine-phase LBA range summary is still missing
Line 1382 adds dump-mode range output, but refine mode (Lines 1388-1393) still never prints remaining bad LBA ranges. Since
error_intervalsis only filled underif(dump), refine has no range-tracking source, so the “print again after refine phase” objective is not met.Suggested direction
- IntervalSet<int32_t> error_intervals; + IntervalSet<int32_t> error_intervals; + IntervalSet<int32_t> refine_error_intervals; ... - else - { - if(options.verbose) - LOGC_R("[LBA: {} .. {}] correction failure", lba, lba + (int32_t)sectors_to_read - 1); - } + else + { + refine_error_intervals.add(lba, lba + (int32_t)sectors_to_read); + if(options.verbose) + LOGC_R("[LBA: {} .. {}] correction failure", lba, lba + (int32_t)sectors_to_read - 1); + } ... else { LOG("correction statistics: "); LOG(" SCSI: {}", errors_initial.scsi - errors.scsi); LOG(" EDC: {}", errors_initial.edc - errors.edc); + if(options.verbose && !refine_error_intervals.empty()) + { + LOG(""); + LOG("LBA error ranges: {}", refine_error_intervals.to_string()); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dvd/dvd_dump.ixx` around lines 1382 - 1393, The refine-phase summary still omits remaining bad LBA ranges because error_intervals is only populated in the dump path. Update the refine-phase logic around the existing error_intervals and correction statistics block so the range tracker is also maintained when refining, then emit the remaining LBA ranges after the refine pass just like the dump-mode summary. Keep the change localized to the code that currently gates range collection on dump and the final LOG output that prints correction statistics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@dvd/dvd_dump.ixx`:
- Around line 1382-1393: The refine-phase summary still omits remaining bad LBA
ranges because error_intervals is only populated in the dump path. Update the
refine-phase logic around the existing error_intervals and correction statistics
block so the range tracker is also maintained when refining, then emit the
remaining LBA ranges after the refine pass just like the dump-mode summary. Keep
the change localized to the code that currently gates range collection on dump
and the final LOG output that prints correction statistics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dcff2f0-3444-49b0-ae3b-ecc5ffb6d464
📒 Files selected for processing (3)
cd/cd_dump.ixxdebug.ixxdvd/dvd_dump.ixx
🚧 Files skipped from review as they are similar to previous changes (2)
- cd/cd_dump.ixx
- debug.ixx
superg
left a comment
superg
left a comment
There was a problem hiding this comment.
This is questionable, I don't know what to do with this.
| } | ||
|
|
||
|
|
||
| std::string to_string() const |
There was a problem hiding this comment.
This function doesn't belong here, it's fairly custom formatter that includes last element into range and quite ineffective implementation. Please move it to the place of use.
| LOG(" C2: {} samples", errors.c2); | ||
| LOG(" Q: {}", errors.q); | ||
|
|
||
| if(options.verbose && (!scsi_error_intervals.empty() || !c2_error_intervals.empty())) |
There was a problem hiding this comment.
This will not help at all, if disc is damaged, usually you get thousands of C2 errors and it's not every sector so this output will be a mess of tiny ranges.
I don't want this in log.
There was a problem hiding this comment.
Verbose printing of errors to log is requested by multiple mods and this error summary is the compromise I could come up with
There was a problem hiding this comment.
Can you enumerate cases where do we want to see errors and how does accept / reject submission depend on this?
Maybe there is a better way.
There was a problem hiding this comment.
This printing is under "verbose" and should not be printed for most users. This printing is for people who want to see all the error locations, and is not relevant for redump.info
| LOG(" SCSI: {}", errors.scsi); | ||
| LOG(" EDC: {}", errors.edc); | ||
|
|
||
| if(options.verbose && !error_intervals.empty()) |
There was a problem hiding this comment.
Not as bad as CD but I think it's useless.
| return exit_code; | ||
| } | ||
|
|
||
| export int redumper_state(Context &ctx, Options &options) |
There was a problem hiding this comment.
Is this whole thing vibecoded?
Looks pretty low effort and too many things to correct.
There was a problem hiding this comment.
If you don't want a debug command I can remove it, it's just for mods who want to parse an existing state file
There was a problem hiding this comment.
I want great code but it's just dirty implementation.
| return exit_code; | ||
| } | ||
|
|
||
| export int redumper_state(Context &ctx, Options &options) |
There was a problem hiding this comment.
I want great code but it's just dirty implementation.
| if(!cd && options.disc_type && *options.disc_type == "CD") | ||
| cd = true; |
There was a problem hiding this comment.
Why this check at all?
.scram exists, that's cd, .sdram exists it's dvd, .sbram exists it's bluray.
There was a problem hiding this comment.
So that you can print the state file from someone else's logs, you can manually say it's CD state file
| if(!state_fs.is_open()) | ||
| throw_line("unable to open file ({})", state_path.filename().string()); | ||
|
|
||
| int32_t offset = 0; |
There was a problem hiding this comment.
Is this any useful if it's not aligned to sync for data disc?
I think most users will be confused by this.
There was a problem hiding this comment.
I think the need for the function is just to help visualize what is in the state file. You say that there is no need to print to log but they still want the error location information including C2 error sizes. It is already in state file so this debug function is meant to provide that function.
| else if(std::filesystem::exists(image_prefix + ".sbram")) | ||
| offset = bd::LBA_START; | ||
|
|
||
| static const char *STATE_NAME[] = { "Unread", "Read Error", "Success (Plextor lead-in)", "Success (MediaTek lead-out)", "Success" }; |
There was a problem hiding this comment.
I don't know why call that Plextor lead-in and MediaTek lead-out, those states aren't called that at all.
One is no confirmed C2 data, the other one is no SCSI state.
There was a problem hiding this comment.
That's the only time those states are ever used in redumper code that I could see
|
|
||
| LOG("state file (unit: {}): ", cd ? "samples" : "sectors"); | ||
|
|
||
| constexpr uint32_t CHUNK = 4096; |
There was a problem hiding this comment.
Any reason for specific 4Kb? There is CHUNK_1KB and CHUNK_1MB in common.ixx, I like to have that centralized.
|
|
||
| for(uint64_t i = 0; i < entries_count; i += CHUNK) | ||
| { | ||
| uint32_t count = std::min((uint64_t)CHUNK, entries_count - i); |
There was a problem hiding this comment.
I'm really confused by this math, aren't you missing the last element?
| LOG(" {}..{}: {}", range_start, range_end, state_name); | ||
| }; | ||
|
|
||
| for(uint64_t i = 0; i < entries_count; i += CHUNK) |
There was a problem hiding this comment.
I believe this loop can be organized in a way that you will not have to call print_range twice but once.
| State current = State::ERROR_SKIP; | ||
| int64_t range_start = offset; | ||
|
|
||
| auto print_range = [&](int64_t range_end) |
There was a problem hiding this comment.
I'd prefer not to have this lambda, see next comment.
2 participants
Requested changes now that --verbose does not spam the log output file
debug::statecommand for Morlit and others who want a visual representation of the state fileSummary by CodeRabbit