[thin_restore (rust)] Fix errors in tests

- Move error messages to stderr (fixes unwritable_output_file,
  input_file_not_found, and missing_input_option)
- Validate XML structures implicitly (fixes corrupted_input_data)
- Check output file size (fixes tiny_output_file)
- Provide XML error context
- Fix mk_valid_input()

src/bin/thin_restore.rs

@@ -62,6 +62,11 @@ fn main() {
         exit(1);
     }
 
+    if let Err(e) = file_utils::check_output_file_requirements(output_file) {
+        eprintln!("{}", e);
+        exit(1);
+    }
+
     let report;
 
     if matches.is_present("QUIET") {
@@ -80,7 +85,7 @@ fn main() {
     };
 
     if let Err(reason) = restore(opts) {
-        println!("{}", reason);
+        eprintln!("{}", reason);
         process::exit(1);
     }
 }

src/file_utils.rs

@@ -98,3 +98,13 @@ pub fn create_sized_file(path: &Path, nr_bytes: u64) -> io::Result<std::fs::File
 }
 
 //---------------------------------------
+
+pub fn check_output_file_requirements(path: &Path) -> io::Result<()> {
+    // minimal thin metadata size is 10 blocks, with one device
+    if file_size(path)? < 40960 {
+        return fail("Output file too small.");
+    }
+    Ok(())
+}
+
+//---------------------------------------

src/thin/restore.rs

@@ -57,6 +57,15 @@ impl std::fmt::Display for MappedSection {
 
 //------------------------------------------
 
+#[derive(PartialEq)]
+enum Section {
+    None,
+    Superblock,
+    Device,
+    Def,
+    Finalized,
+}
+
 pub struct Restorer<'a> {
     w: &'a mut WriteBatcher,
     report: Arc<Report>,
@@ -71,6 +80,7 @@ pub struct Restorer<'a> {
     sb: Option<ir::Superblock>,
     devices: BTreeMap<u32, (DeviceDetail, u64)>,
     data_sm: Option<Arc<Mutex<dyn SpaceMap>>>,
+    in_section: Section,
 }
 
 impl<'a> Restorer<'a> {
@@ -84,6 +94,7 @@ impl<'a> Restorer<'a> {
             sb: None,
             devices: BTreeMap::new(),
             data_sm: None,
+            in_section: Section::None,
         }
     }
 
@@ -149,6 +160,13 @@ impl<'a> Restorer<'a> {
     }
 
     fn finalize(&mut self) -> Result<()> {
+        let src_sb;
+        if let Some(sb) = self.sb.take() {
+            src_sb = sb;
+        } else {
+            return Err(anyhow!("missing superblock"));
+        }
+
         let (details_root, mapping_root) = self.build_device_details()?;
 
         self.release_subtrees()?;
@@ -162,33 +180,41 @@ impl<'a> Restorer<'a> {
         let metadata_sm_root = pack_root(&metadata_sm, SPACE_MAP_ROOT_SIZE)?;
 
         // Write the superblock
-        let sb = self.sb.as_ref().unwrap();
         let sb = superblock::Superblock {
             flags: SuperblockFlags { needs_check: false },
             block: SUPERBLOCK_LOCATION,
             version: 2,
-            time: sb.time as u32,
-            transaction_id: sb.transaction,
+            time: src_sb.time as u32,
+            transaction_id: src_sb.transaction,
             metadata_snap: 0,
             data_sm_root,
             metadata_sm_root,
             mapping_root,
             details_root,
-            data_block_size: sb.data_block_size,
+            data_block_size: src_sb.data_block_size,
             nr_metadata_blocks: metadata_sm.nr_blocks,
         };
-        write_superblock(self.w.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)
+        write_superblock(self.w.engine.as_ref(), SUPERBLOCK_LOCATION, &sb)?;
+        self.in_section = Section::Finalized;
+
+        Ok(())
     }
 }
 
 impl<'a> MetadataVisitor for Restorer<'a> {
     fn superblock_b(&mut self, sb: &ir::Superblock) -> Result<Visit> {
+        if self.in_section != Section::None {
+            return Err(anyhow!("duplicated superblock"));
+        }
+
         self.sb = Some(sb.clone());
         self.data_sm = Some(core_sm(sb.nr_data_blocks, u32::MAX));
         let b = self.w.alloc()?;
         if b.loc != SUPERBLOCK_LOCATION {
             return Err(anyhow!("superblock was occupied"));
         }
+        self.in_section = Section::Superblock;
+
         Ok(Visit::Continue)
     }
 
@@ -198,12 +224,17 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn def_shared_b(&mut self, name: &str) -> Result<Visit> {
+        if self.in_section != Section::Superblock {
+            return Err(anyhow!("missing superblock"));
+        }
+        self.in_section = Section::Def;
         self.begin_section(MappedSection::Def(name.to_string()))
     }
 
     fn def_shared_e(&mut self) -> Result<Visit> {
         if let (MappedSection::Def(name), nodes) = self.end_section()? {
             self.sub_trees.insert(name, nodes);
+            self.in_section = Section::Superblock;
             Ok(Visit::Continue)
         } else {
             Err(anyhow!("unexpected </def>"))
@@ -211,6 +242,9 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn device_b(&mut self, d: &ir::Device) -> Result<Visit> {
+        if self.in_section != Section::Superblock {
+            return Err(anyhow!("missing superblock"));
+        }
         self.report
             .info(&format!("building btree for device {}", d.dev_id));
         self.current_dev = Some(DeviceDetail {
@@ -219,6 +253,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
             creation_time: d.creation_time as u32,
             snapshotted_time: d.snap_time as u32,
         });
+        self.in_section = Section::Device;
         self.begin_section(MappedSection::Dev(d.dev_id))
     }
 
@@ -227,6 +262,7 @@ impl<'a> MetadataVisitor for Restorer<'a> {
             if let (MappedSection::Dev(thin_id), nodes) = self.end_section()? {
                 let root = build_btree(self.w, nodes)?;
                 self.devices.insert(thin_id, (detail, root));
+                self.in_section = Section::Superblock;
                 Ok(Visit::Continue)
             } else {
                 Err(anyhow!("internal error, couldn't find device details"))
@@ -278,7 +314,9 @@ impl<'a> MetadataVisitor for Restorer<'a> {
     }
 
     fn eof(&mut self) -> Result<Visit> {
-        // FIXME: build the rest of the device trees
+        if self.in_section != Section::Finalized {
+            return Err(anyhow!("incomplete source metadata"));
+        }
         Ok(Visit::Continue)
     }
 }

src/thin/xml.rs

@@ -1,4 +1,4 @@
-use anyhow::Result;
+use anyhow::{anyhow, Result};
 use std::{io::prelude::*, io::BufReader, io::Write};
 
 use quick_xml::events::{BytesEnd, BytesStart, Event};
@@ -271,19 +271,19 @@ where
             b"superblock" => visitor.superblock_b(&parse_superblock(e)?),
             b"device" => visitor.device_b(&parse_device(e)?),
             b"def" => visitor.def_shared_b(&parse_def(e, "def")?),
-            _ => todo!(),
+            _ => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())),
         },
         Ok(Event::End(ref e)) => match e.name() {
             b"superblock" => visitor.superblock_e(),
             b"device" => visitor.device_e(),
             b"def" => visitor.def_shared_e(),
-            _ => todo!(),
+            _ => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())),
         },
         Ok(Event::Empty(ref e)) => match e.name() {
             b"single_mapping" => visitor.map(&parse_single_map(e)?),
             b"range_mapping" => visitor.map(&parse_range_map(e)?),
             b"ref" => visitor.ref_shared(&parse_def(e, "ref")?),
-            _ => todo!(),
+            _ => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())),
         },
         Ok(Event::Text(_)) => Ok(Visit::Continue),
         Ok(Event::Comment(_)) => Ok(Visit::Continue),
@@ -291,10 +291,14 @@ where
             visitor.eof()?;
             Ok(Visit::Stop)
         }
-        Ok(_) => todo!(),
-
-        // FIXME: don't panic!
-        Err(e) => panic!("error parsing xml {:?}", e),
+        Ok(_) => return Err(anyhow!("Parse error at byte {}", reader.buffer_position())),
+        Err(e) => {
+            return Err(anyhow!(
+                "Parse error at byte {}: {:?}",
+                reader.buffer_position(),
+                e
+            ))
+        }
     }
 }
 

src/xml.rs

@@ -32,8 +32,8 @@ pub fn bool_val(kv: &Attribute) -> anyhow::Result<bool> {
     Ok(n)
 }
 
-pub fn bad_attr<T>(_tag: &str, _attr: &[u8]) -> anyhow::Result<T> {
-    todo!();
+pub fn bad_attr<T>(tag: &str, _attr: &[u8]) -> anyhow::Result<T> {
+    Err(anyhow!("unknown attribute in tag '{}'", tag))
 }
 
 pub fn check_attr<T>(tag: &str, name: &str, maybe_v: Option<T>) -> anyhow::Result<T> {

tests/thin_restore.rs

@@ -53,7 +53,7 @@ impl<'a> Program<'a> for ThinRestore {
 
 impl<'a> InputProgram<'a> for ThinRestore {
     fn mk_valid_input(td: &mut TestDir) -> Result<std::path::PathBuf> {
-        mk_valid_md(td)
+        mk_valid_xml(td)
     }
 
     fn file_not_found() -> &'a str {