Better default for raw DVD dump read size#351
Conversation
📝 WalkthroughWalkthroughRemoved exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dvd/dvd_dump.ixx (1)
834-834:omnidrive_firmwareis redundant in the condition.
rawis defined as(options.dvd_raw || nintendo) && omnidrive_firmware(line 827), soraw == truealready guaranteesomnidrive_firmware == true. The extra&& omnidrive_firmwarein the guard is a no-op.♻️ Proposed simplification
- if(raw && omnidrive_firmware && sectors_at_once == 32) + if(raw && sectors_at_once == 32)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` at line 834, The condition redundantly checks omnidrive_firmware because raw is already defined as (options.dvd_raw || nintendo) && omnidrive_firmware; update the guard in the if statement that currently reads if(raw && omnidrive_firmware && sectors_at_once == 32) to remove the redundant omnidrive_firmware check so it becomes if(raw && sectors_at_once == 32), leaving raw and sectors_at_once unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Line 834: The condition redundantly checks omnidrive_firmware because raw is
already defined as (options.dvd_raw || nintendo) && omnidrive_firmware; update
the guard in the if statement that currently reads if(raw && omnidrive_firmware
&& sectors_at_once == 32) to remove the redundant omnidrive_firmware check so it
becomes if(raw && sectors_at_once == 32), leaving raw and sectors_at_once
unchanged.
dvd/dvd_dump.ixx
Outdated
|
|
||
| uint32_t sectors_at_once = (dump_mode == DumpMode::REFINE ? 1 : options.dump_read_size); | ||
| if(raw && is_omnidrive_slim(ctx.drive_config) && sectors_at_once >= 32) | ||
| if(raw && omnidrive_firmware && sectors_at_once == 32) |
There was a problem hiding this comment.
I want to allow the dumper to use >31 as it will work with ASUS via SATA. Only the default should be 31.
I can't check that the dumper set a value or not, I can only check that it equals to the default.
There was a problem hiding this comment.
That's too complicated to handle.
Let's change options.dump_read_size
from
int dump_read_size;
to
std::unique_ptr<int> dump_read_size;
remove the default 32 from options
and have it defined as constexpr in common.ixx in the constexpr block:
export constexpr uint32_t DVD_READ_SIZE = 32;
and then if option not null, use it, else if null check if omnidrive, 31, if not assign default.
There was a problem hiding this comment.
Yeah let's keep the warning like it was if 31 was used.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
options.ixx (1)
451-451: Help text no longer shows a default — consider mentioning the contextual defaults.Since the effective default now varies (31 for raw DVD with OmniDrive firmware, 32 otherwise), it's reasonable to drop the static default from the help text. However, users might benefit from a brief note about the contextual behavior, e.g.,
"(default: 32, 31 for raw DVD)".Suggested help text improvement
- LOG("\t--dump-read-size=VALUE \tnumber of sectors to read at once on initial dump, DVD only"); + LOG("\t--dump-read-size=VALUE \tnumber of sectors to read at once on initial dump, DVD only (default: 32, 31 for raw DVD)");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@options.ixx` at line 451, The help text for the dump-read-size option currently omits any default information; update the LOG call that emits the string containing "--dump-read-size=VALUE" (the LOG invocation at options.ixx) to append a concise contextual-default note such as "(default: 32, 31 for raw DVD)" so users see the effective defaults; locate the LOG(...) that prints "\t--dump-read-size=VALUE ..." and modify the message to include the suggested parenthetical describing the two-context default behavior.dvd/dvd_dump.ixx (1)
833-841: The&& omnidrive_firmwarecheck on line 836 is redundant.Given line 827 (
bool raw = (options.dvd_raw || nintendo) && omnidrive_firmware), wheneverrawistrue,omnidrive_firmwareis necessarilytrue. The extra condition is a no-op.Simplify the condition
uint32_t dump_read_size; if(options.dump_read_size) dump_read_size = *options.dump_read_size; - else if(raw && omnidrive_firmware) + else if(raw) dump_read_size = 31; // default total transfer length less than 65536 bytes else dump_read_size = DVD_READ_SIZE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 833 - 841, The else-if condition checking "raw && omnidrive_firmware" is redundant because raw is already defined as "(options.dvd_raw || nintendo) && omnidrive_firmware"; update the branch that sets dump_read_size to use "else if (raw)" instead of "else if (raw && omnidrive_firmware)" so the logic remains identical but simpler; ensure you update the surrounding block that computes dump_read_size (variables: dump_read_size, options.dump_read_size, raw, omnidrive_firmware) and verify sectors_at_once calculation using DumpMode::REFINE is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dvd/dvd_dump.ixx`:
- Around line 833-835: Validate the user-supplied options.dump_read_size before
assigning to dump_read_size: ensure the value is present and strictly positive
(reject zero), and guard against negative inputs (don't cast negative ints to
uint32_t); also cap the value to a reasonable maximum (e.g., not larger than the
remaining sectors_to_read or a predefined MAX_DUMP_READ_SIZE) to avoid huge
allocations and infinite loops where lba += dump_read_size. Update the
assignment logic around dump_read_size to check options.dump_read_size for null,
verify *options.dump_read_size > 0 and within the allowed maximum, and handle
invalid values by returning an error or falling back to a safe default; adjust
downstream code that uses dump_read_size (sectors_to_read, lba, and the buffer
allocation) to rely on the validated value.
---
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Around line 833-841: The else-if condition checking "raw &&
omnidrive_firmware" is redundant because raw is already defined as
"(options.dvd_raw || nintendo) && omnidrive_firmware"; update the branch that
sets dump_read_size to use "else if (raw)" instead of "else if (raw &&
omnidrive_firmware)" so the logic remains identical but simpler; ensure you
update the surrounding block that computes dump_read_size (variables:
dump_read_size, options.dump_read_size, raw, omnidrive_firmware) and verify
sectors_at_once calculation using DumpMode::REFINE is unchanged.
In `@options.ixx`:
- Line 451: The help text for the dump-read-size option currently omits any
default information; update the LOG call that emits the string containing
"--dump-read-size=VALUE" (the LOG invocation at options.ixx) to append a concise
contextual-default note such as "(default: 32, 31 for raw DVD)" so users see the
effective defaults; locate the LOG(...) that prints "\t--dump-read-size=VALUE
..." and modify the message to include the suggested parenthetical describing
the two-context default behavior.
| uint32_t dump_read_size; | ||
| if(options.dump_read_size) | ||
| dump_read_size = *options.dump_read_size; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the code at lines 833-835 and surrounding context
sed -n '825,850p' dvd/dvd_dump.ixxRepository: superg/redumper
Length of output: 1033
🏁 Script executed:
# Trace dump_read_size usage throughout the file
rg -n 'dump_read_size' dvd/dvd_dump.ixx -B2 -A2Repository: superg/redumper
Length of output: 609
🏁 Script executed:
# Check if validation exists elsewhere for dump_read_size or similar options
rg -n 'options\.dump_read_size' --type=cpp -B3 -A3Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Look at how sectors_at_once is used (line 843 mentioned in comment)
rg -n 'sectors_at_once' dvd/dvd_dump.ixx -B2 -A2Repository: superg/redumper
Length of output: 737
🏁 Script executed:
# Check other integer option validations in the codebase
rg -n 'throw_line.*must be' --type=cpp -A1 -B1Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Find the definition of options.dump_read_size
rg -n 'dump_read_size' --type=cpp --type=h | head -30Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Search for command-line argument parsing for dump-read-size
rg -n 'dump-read-size|dump_read_size' --type=cpp -B3 -A3 | head -60Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Check what DVD_READ_SIZE is set to
rg -n 'DVD_READ_SIZE' --type=cpp -B2 -A2Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Look at the Options struct definition
fd -e h | xargs rg -l 'dump_read_size' | head -5Repository: superg/redumper
Length of output: 87
🏁 Script executed:
# Check if there's validation in argument parsing or option handling
rg -n 'argv|argparse|getopt|option' dvd/dvd_dump.ixx | head -20Repository: superg/redumper
Length of output: 1437
🏁 Script executed:
# Find and examine options.ixx to see dump_read_size definition
fd options.ixx | head -5Repository: superg/redumper
Length of output: 70
🏁 Script executed:
# Search for dump_read_size in options.ixx
rg -n 'dump_read_size' options.ixx -B5 -A5Repository: superg/redumper
Length of output: 1073
🏁 Script executed:
# Look for command-line argument parsing/validation in the codebase
rg -n 'main|argc|argv' --type=cpp -l | head -5Repository: superg/redumper
Length of output: 81
🏁 Script executed:
# Check for any validation logic that might apply to dump_read_size
rg -n 'dump.read.size|dump_read' --type=cpp -i -B3 -A3Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Look at the actual size of file_data allocation to understand the impact
sed -n '833,920p' dvd/dvd_dump.ixx | grep -A 50 'dump_read_size'Repository: superg/redumper
Length of output: 1839
🏁 Script executed:
# Check what happens with sectors_at_once in the read loop
sed -n '900,950p' dvd/dvd_dump.ixxRepository: superg/redumper
Length of output: 1902
🏁 Script executed:
# Look at the read loop logic to see how sectors_at_once is used
sed -n '905,920p' dvd/dvd_dump.ixx | cat -nRepository: superg/redumper
Length of output: 743
🏁 Script executed:
# Check for any defensive coding around sectors_at_once or sectors_to_read
rg -n 'sectors_to_read|sectors_at_once' dvd/dvd_dump.ixx -B2 -A2Repository: superg/redumper
Length of output: 5426
🏁 Script executed:
# Look at dump_write_offset which is similar (also int to uint32_t) to see if it's validated
rg -n 'dump_write_offset' dvd/dvd_dump.ixx -B3 -A3Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Check main.cc to see if validation happens at the parsing layer
sed -n '1,50p' main.ccRepository: superg/redumper
Length of output: 978
🏁 Script executed:
# Check how dump_write_offset is used (similar int->uint32_t conversion)
sed -n '1,100p' dvd/dvd_dump.ixx | grep -A 10 'dump_write_offset'Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Look for dump_write_offset usage in the file
rg -n 'dump_write_offset' dvd/dvd_dump.ixx -B3 -A3Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Search for where int options are converted to uint32_t or used in allocations
sed -n '800,900p' dvd/dvd_dump.ixx | grep -E '(int|uint32_t|options\.)' | head -30Repository: superg/redumper
Length of output: 1299
🏁 Script executed:
# Check if there are any constraints on what DVD_READ_SIZE is
rg -n 'DVD_READ_SIZE.*=' --type=cpp -B1 -A1Repository: superg/redumper
Length of output: 41
🏁 Script executed:
# Check if there's a minimum value for sectors_at_once anywhere or if 0 would cause an infinite loop
sed -n '900,950p' dvd/dvd_dump.ixx | grep -E 'lba \+|sectors_to_read'Repository: superg/redumper
Length of output: 642
🏁 Script executed:
# Look at what happens when sectors_to_read = 0 in the loop
sed -n '1080,1095p' dvd/dvd_dump.ixxRepository: superg/redumper
Length of output: 430
🏁 Script executed:
# Check the structure of the main dump loop to see what happens with 0 sectors
sed -n '905,915p' dvd/dvd_dump.ixxRepository: superg/redumper
Length of output: 476
🏁 Script executed:
# Look for any existing validation patterns in the same function for similar parameters
sed -n '865,880p' dvd/dvd_dump.ixxRepository: superg/redumper
Length of output: 409
🏁 Script executed:
# Check what cfg.sector_size is to understand the allocation impact
rg -n 'sector_size' dvd/dvd_dump.ixx -B1 -A1 | head -20Repository: superg/redumper
Length of output: 811
🏁 Script executed:
# Check if sectors_at_once = 0 would cause infinite loop by examining loop increment
sed -n '905,1095p' dvd/dvd_dump.ixx | grep -E '(for.*lba|lba \+=|sectors_to_read)'Repository: superg/redumper
Length of output: 2033
🏁 Script executed:
# Verify if dump_read_size could realistically be 0 or negative by checking option parsing
sed -n '280,310p' options.ixxRepository: superg/redumper
Length of output: 1610
🏁 Script executed:
# Check if there are any guards that might prevent 0 value from being used
rg -n 'sectors_at_once.*=' dvd/dvd_dump.ixx -B2 -A5Repository: superg/redumper
Length of output: 429
🏁 Script executed:
# Examine the context around the allocation to see if there's any defensive programming
sed -n '840,860p' dvd/dvd_dump.ixxRepository: superg/redumper
Length of output: 854
Add bounds validation for user-supplied dump_read_size.
*options.dump_read_size converts int to uint32_t without validation. Passing --dump-read-size=0 results in an infinite loop (sectors_to_read stays 0, lba += 0 never increments). Passing a negative value wraps to a huge uint32_t, causing memory allocation failure on line 843.
Suggested validation
uint32_t dump_read_size;
if(options.dump_read_size)
+ {
+ if(*options.dump_read_size < 1)
+ throw_line("dump-read-size must be a positive integer");
dump_read_size = *options.dump_read_size;
+ }
else if(raw && omnidrive_firmware)
dump_read_size = 31; // default total transfer length less than 65536 bytes
else
dump_read_size = DVD_READ_SIZE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dvd/dvd_dump.ixx` around lines 833 - 835, Validate the user-supplied
options.dump_read_size before assigning to dump_read_size: ensure the value is
present and strictly positive (reject zero), and guard against negative inputs
(don't cast negative ints to uint32_t); also cap the value to a reasonable
maximum (e.g., not larger than the remaining sectors_to_read or a predefined
MAX_DUMP_READ_SIZE) to avoid huge allocations and infinite loops where lba +=
dump_read_size. Update the assignment logic around dump_read_size to check
options.dump_read_size for null, verify *options.dump_read_size > 0 and within
the allowed maximum, and handle invalid values by returning an error or falling
back to a safe default; adjust downstream code that uses dump_read_size
(sectors_to_read, lba, and the buffer allocation) to rely on the validated
value.
There was a problem hiding this comment.
@superg how would you like to handle this case? It's not new from my changes
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Safe code would be something like:
if(*options.dump_read_size <= 0)
throw_line("dump read size must be positive (value: {})", *options.dump_read_size);
dump_read_size = *options.dump_read_size;
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dvd/dvd_dump.ixx (2)
840-840:&& omnidrive_firmwareis redundant.
raw(line 827) is already defined as(options.dvd_raw || nintendo) && omnidrive_firmware, soraw == trueimpliesomnidrive_firmware == true. The compound condition simplifies toelse if(raw).♻️ Proposed simplification
- else if(raw && omnidrive_firmware) + else if(raw)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` at line 840, The condition `else if(raw && omnidrive_firmware)` is redundant because `raw` is defined as `(options.dvd_raw || nintendo) && omnidrive_firmware`, so `raw` already implies `omnidrive_firmware`; change the branch to `else if (raw)` in the same scope (referencing the `raw` variable and `omnidrive_firmware` symbol) to simplify the logic and remove the unnecessary boolean check, and update any nearby comment or docstring if it mentions the redundant guard.
842-844: "warning:" prefix is misleading for the intended default path; also fires spuriously in REFINE mode.Two issues:
- Labelling the new default behavior as
"warning:"signals an unexpected problem to users when 31 is in fact the deliberate, expected default for raw OmniDrive dumps.- In
DumpMode::REFINE,dump_read_sizeis set to 31 here butsectors_at_onceis immediately overridden to1(line 849), so the message is inaccurate for that mode.Consider gating on dump mode and/or using a plain
LOG:♻️ Suggested refinement
- // ensure default total transfer length is less than 65536 bytes (31 * 2064) - LOG("warning: setting dump read size to 31 for raw dumping"); - dump_read_size = 31; + dump_read_size = 31; // ensure total transfer length < 65536 bytes (31 * 2064) + if(dump_mode != DumpMode::REFINE) + LOG("dump read size: {} (raw DVD default)", dump_read_size);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 842 - 844, The log message is misleading and fires in REFINE mode where sectors_at_once is immediately set to 1; change the behavior around dump_read_size and logging in the block that sets dump_read_size and sectors_at_once: set dump_read_size = 31 as the deliberate default for raw dumping, remove the "warning:" prefix and replace with a neutral LOG message (e.g. "setting default dump read size to 31") or only emit a message when the chosen read size will actually be used (i.e. guard the LOG with a check of DumpMode != DumpMode::REFINE or that sectors_at_once > 1), and ensure sectors_at_once is not reported as 31 when it will be overridden—adjust the order or the conditional so the message reflects the effective mode and values (referencing dump_read_size, DumpMode::REFINE, sectors_at_once, and LOG).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dvd/dvd_dump.ixx`:
- Around line 836-838: The code currently validates options.dump_read_size only
for >0 but allows arbitrarily large values which can cause huge heap allocations
when multiplied by cfg.sector_size; update the check around
options.dump_read_size so it enforces an upper bound (e.g. <= DVD_READ_SIZE * 8
or another protocol-appropriate ceiling) and throw_line if the value is out of
range, then assign dump_read_size; reference options.dump_read_size,
dump_read_size and the throw_line used for the error so the new validation
mirrors the existing pattern.
---
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Line 840: The condition `else if(raw && omnidrive_firmware)` is redundant
because `raw` is defined as `(options.dvd_raw || nintendo) &&
omnidrive_firmware`, so `raw` already implies `omnidrive_firmware`; change the
branch to `else if (raw)` in the same scope (referencing the `raw` variable and
`omnidrive_firmware` symbol) to simplify the logic and remove the unnecessary
boolean check, and update any nearby comment or docstring if it mentions the
redundant guard.
- Around line 842-844: The log message is misleading and fires in REFINE mode
where sectors_at_once is immediately set to 1; change the behavior around
dump_read_size and logging in the block that sets dump_read_size and
sectors_at_once: set dump_read_size = 31 as the deliberate default for raw
dumping, remove the "warning:" prefix and replace with a neutral LOG message
(e.g. "setting default dump read size to 31") or only emit a message when the
chosen read size will actually be used (i.e. guard the LOG with a check of
DumpMode != DumpMode::REFINE or that sectors_at_once > 1), and ensure
sectors_at_once is not reported as 31 when it will be overridden—adjust the
order or the conditional so the message reflects the effective mode and values
(referencing dump_read_size, DumpMode::REFINE, sectors_at_once, and LOG).
31 dumps much faster than 16, even though it's an annoying number.
Half-height drives (ASUS) still have an issue with 32 dump read size when used with some adapters, so we have to default to 31 for those too.
It would be nice to not fallback to 31 if the dumper specifically requested
--dump-read-size=32, but there's no way with the way options currently work.Summary by CodeRabbit
Bug Fixes
New Features
Documentation