Skip to content

Commit 3c6d9c6

Browse files
committed
Fix PR review comments
1 parent 4f06f2b commit 3c6d9c6

5 files changed

Lines changed: 92 additions & 36 deletions

File tree

src/create/file_naming.rs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,16 @@ fn allocate_recovery_blocks(
122122
};
123123
}
124124

125-
assert!(blocks > 0 && file_number > 0);
125+
if blocks == 0 || file_number == 0 {
126+
return allocate_recovery_blocks(
127+
recovery_file_count,
128+
recovery_block_count,
129+
first_recovery_block,
130+
RecoveryFileScheme::Uniform,
131+
largest_file_size,
132+
block_size,
133+
);
134+
}
126135

127136
exponent = first_recovery_block;
128137
let mut count = 1;
@@ -375,6 +384,17 @@ mod tests {
375384
assert_eq!(allocations[5].count, 0);
376385
}
377386

387+
#[test]
388+
fn limited_scheme_falls_back_when_cap_exhausts_files() {
389+
let allocations = allocate_recovery_blocks(1, 10, 0, RecoveryFileScheme::Limited, 4, 1);
390+
391+
assert_eq!(allocations.len(), 2);
392+
assert_eq!(allocations[0].exponent, 0);
393+
assert_eq!(allocations[0].count, 10);
394+
assert_eq!(allocations[1].exponent, 10);
395+
assert_eq!(allocations[1].count, 0);
396+
}
397+
378398
/// Test count_digits helper
379399
/// Reference: par2cmdline-turbo/src/par2creator.cpp lines 604-615
380400
#[test]

src/packets/mod.rs

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,22 @@ fn scan_for_next_magic<R: Read>(reader: &mut R) -> std::io::Result<Option<[u8; 8
229229
}
230230
}
231231

232+
fn resync_to_next_magic<R: Read + Seek>(reader: &mut R, rewind_bytes: i64) -> bool {
233+
if rewind_bytes != 0
234+
&& reader
235+
.seek(std::io::SeekFrom::Current(-rewind_bytes))
236+
.is_err()
237+
{
238+
return false;
239+
}
240+
241+
if scan_for_next_magic(reader).ok().flatten().is_none() {
242+
return false;
243+
}
244+
245+
reader.seek(std::io::SeekFrom::Current(-8)).is_ok()
246+
}
247+
232248
/// Parse packets with optional recovery slice inclusion
233249
///
234250
/// When `include_recovery_slices` is false, recovery slice packet headers are still
@@ -261,12 +277,9 @@ pub fn parse_packets_with_options<R: Read + Seek>(
261277
break;
262278
}
263279
Err(PacketParseError::InvalidMagic(_)) => {
264-
// Bad magic - try to find next valid packet by scanning forward
265-
if scan_for_next_magic(reader).ok().flatten().is_some() {
266-
// Found magic, but we need to rewind 8 bytes so the next parse reads the header
267-
if reader.seek(std::io::SeekFrom::Current(-8)).is_err() {
268-
break;
269-
}
280+
// PacketHeader::parse consumed 64 bytes. Rewind 63 bytes so
281+
// resync still checks the byte immediately after the bad start.
282+
if resync_to_next_magic(reader, 63) {
270283
continue;
271284
} else {
272285
break;
@@ -285,12 +298,7 @@ pub fn parse_packets_with_options<R: Read + Seek>(
285298
}
286299
Err(_) => {
287300
// Validation failed - try to find next valid packet
288-
if scan_for_next_magic(reader).ok().flatten().is_some() {
289-
// Found magic, rewind 8 bytes
290-
if reader.seek(std::io::SeekFrom::Current(-8)).is_err() {
291-
break;
292-
}
293-
} else {
301+
if !resync_to_next_magic(reader, 0) {
294302
break;
295303
}
296304
}
@@ -303,10 +311,7 @@ pub fn parse_packets_with_options<R: Read + Seek>(
303311
Ok(data) => data,
304312
Err(_) => {
305313
// Failed to read packet body - try to find next valid packet
306-
if scan_for_next_magic(reader).ok().flatten().is_some() {
307-
if reader.seek(std::io::SeekFrom::Current(-8)).is_err() {
308-
break;
309-
}
314+
if resync_to_next_magic(reader, 0) {
310315
continue;
311316
} else {
312317
break;
@@ -724,6 +729,20 @@ mod tests {
724729
assert_eq!(pos, 20); // 12 bytes before magic + 8 magic bytes
725730
}
726731

732+
#[test]
733+
fn invalid_magic_resync_checks_next_byte() {
734+
let mut data = vec![0xFF; 64];
735+
data[1..9].copy_from_slice(MAGIC_BYTES);
736+
let mut cursor = Cursor::new(&data);
737+
738+
let result = PacketHeader::parse(&mut cursor);
739+
assert!(matches!(result, Err(PacketParseError::InvalidMagic(_))));
740+
assert_eq!(cursor.position(), 64);
741+
742+
assert!(resync_to_next_magic(&mut cursor, 63));
743+
assert_eq!(cursor.position(), 1);
744+
}
745+
727746
#[test]
728747
fn corrupt_packet_recovery() {
729748
// Test that we can recover from a corrupt packet by finding the next valid magic

src/repair/context.rs

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -197,28 +197,27 @@ impl RepairContext {
197197
/// Purge PAR2 files without a repair context, with optional output.
198198
pub fn purge_par_files_for_with_output(par2_file: &str, show_output: bool) -> Result<()> {
199199
let par2_path = Path::new(par2_file);
200-
let par2_dir = par2_path
200+
let _par2_dir = par2_path
201201
.parent()
202202
.ok_or_else(|| RepairError::InvalidPath(par2_path.to_path_buf()))?;
203203

204204
if show_output {
205205
println!("\nPurge par files.");
206206
}
207207

208-
if let Ok(entries) = std::fs::read_dir(par2_dir) {
209-
for entry in entries.flatten() {
210-
let path = entry.path();
211-
if path
212-
.extension()
213-
.and_then(|ext| ext.to_str())
214-
.is_some_and(|ext| ext.eq_ignore_ascii_case("par2"))
215-
{
216-
delete_file(&path)?;
217-
218-
if show_output {
219-
println!("Remove \"{}\".", entry.file_name().to_string_lossy());
220-
}
221-
}
208+
for path in crate::par2_files::collect_par2_files(par2_path) {
209+
if !path.exists() {
210+
continue;
211+
}
212+
213+
delete_file(&path)?;
214+
215+
if show_output {
216+
let file_name = path
217+
.file_name()
218+
.map(|name| name.to_string_lossy())
219+
.unwrap_or_else(|| path.as_os_str().to_string_lossy());
220+
println!("Remove \"{}\".", file_name);
222221
}
223222
}
224223

tests/test_create_integration.rs

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,19 @@ fn create_test_file(path: &Path, size: usize, pattern: u8) -> std::io::Result<()
2626
fs::write(path, data)
2727
}
2828

29+
/// Helper to create unique 4-byte blocks for tiny-block repair tests.
30+
///
31+
/// Repair tests deliberately use tiny 4-byte blocks. A repeated-byte fixture lets
32+
/// repair tools find duplicate "good" blocks in the damaged source file instead
33+
/// of exercising recovery slices.
34+
fn create_indexed_block_file(path: &Path, block_count: u32) -> std::io::Result<()> {
35+
let mut data = Vec::with_capacity(block_count as usize * 4);
36+
for block in 0..block_count {
37+
data.extend_from_slice(&block.to_le_bytes());
38+
}
39+
fs::write(path, data)
40+
}
41+
2942
/// Helper to run par2cmdline-turbo verify command
3043
fn run_par2_verify(par2_file: &Path) -> std::io::Result<bool> {
3144
let output = Command::new("par2").arg("verify").arg(par2_file).output()?;
@@ -219,8 +232,7 @@ fn test_create_then_corrupt_and_repair_with_par2cmdline() {
219232
let test_file = temp.path().join("test.dat");
220233
let par2_file = temp.path().join("test.par2");
221234

222-
// Create test file
223-
create_test_file(&test_file, 4096, 0xDD).unwrap();
235+
create_indexed_block_file(&test_file, 1024).unwrap();
224236

225237
// Create PAR2 files using our implementation
226238
let reporter = Box::new(par2rs::create::ConsoleCreateReporter::new(true)); // quiet mode
@@ -397,7 +409,7 @@ fn repair_using_only_volume_files_succeeds() {
397409
let test_file = temp.path().join("test.dat");
398410
let par2_file = temp.path().join("test.par2");
399411

400-
create_test_file(&test_file, 4096, 0xEF).unwrap();
412+
create_indexed_block_file(&test_file, 1024).unwrap();
401413

402414
let reporter = Box::new(par2rs::create::ConsoleCreateReporter::new(true));
403415
let mut context = par2rs::create::CreateContextBuilder::new()

tests/test_repair_context.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,8 +450,10 @@ fn test_repair_context_purge_par_files_keeps_backups() {
450450

451451
let par2_file = dir.path().join("test.par2");
452452
let par2_vol = dir.path().join("test.vol0+1.par2");
453+
let foreign_par2 = dir.path().join("foreign.par2");
453454
fs::write(&par2_file, b"dummy par2").unwrap();
454455
fs::write(&par2_vol, b"dummy volume").unwrap();
456+
fs::write(&foreign_par2, b"foreign").unwrap();
455457

456458
let packets = vec![
457459
Packet::Main(create_main_packet(vec![file_id])),
@@ -466,6 +468,7 @@ fn test_repair_context_purge_par_files_keeps_backups() {
466468
assert!(backup_file.exists());
467469
assert!(!par2_file.exists());
468470
assert!(!par2_vol.exists());
471+
assert!(foreign_par2.exists());
469472
}
470473

471474
#[test]
@@ -477,10 +480,12 @@ fn test_repair_context_purge_multiple_par2_files() {
477480
let par2_main = dir.path().join("test.par2");
478481
let par2_vol1 = dir.path().join("test.vol01+02.par2");
479482
let par2_vol2 = dir.path().join("test.vol03+04.par2");
483+
let foreign_par2 = dir.path().join("other.par2");
480484

481485
fs::write(&par2_main, b"main").unwrap();
482486
fs::write(&par2_vol1, b"vol1").unwrap();
483487
fs::write(&par2_vol2, b"vol2").unwrap();
488+
fs::write(&foreign_par2, b"foreign").unwrap();
484489

485490
let packets = vec![
486491
Packet::Main(create_main_packet(vec![file_id])),
@@ -492,10 +497,11 @@ fn test_repair_context_purge_multiple_par2_files() {
492497
let result = context.purge_files(par2_main.to_str().unwrap());
493498
assert!(result.is_ok());
494499

495-
// All PAR2 files should be deleted
500+
// All PAR2 files from the same set should be deleted.
496501
assert!(!par2_main.exists());
497502
assert!(!par2_vol1.exists());
498503
assert!(!par2_vol2.exists());
504+
assert!(foreign_par2.exists());
499505
}
500506

501507
#[test]

0 commit comments

Comments
 (0)