Refactor main.rs and fix clippy lints

Status: WIP

Completed:
* Fixed flagged clippy lints
* Moved qemu(), main.rs::mount() and chroot() to the tools module.
* Moved constants in main.rs to constants.rs (including base packages
  array)
* Renamed Presets struct to PresetsCollection to avoid confusion with
  Preset struct
* Moved main() to the top of main.rs to highlight general logic path
* Added comments and docstrings to some functions
* Removed some uses of `use foo::*` to make the source of imported functions
  and structs clearer

TODO:
* Move remaining code in main.rs to modules (except main())
* Break up create() function in to separate steps
* Log every command run (with arguments) to debug! when verbose flag is used
* Add docstrings for remaining functions and document constants (e.g.
  why noatime is used)
* Remove remaining uses of `use foo::*`
* Consider renaming/moving tools module to address tool:: vs. Tool::
  confusion

src/args.rs

@@ -2,6 +2,8 @@ use byte_unit::Byte;
 use std::path::PathBuf;
 use structopt::StructOpt;
 
+/// Parse size argument as bytes
+/// e.g. 10GB, 10GiB, etc.
 fn parse_bytes(src: &str) -> Result<Byte, &'static str> {
     Byte::from_str(src).map_err(|_| "Invalid image size")
 }

src/constants.rs

@@ -0,0 +1,17 @@
+pub const BOOT_PARTITION_INDEX: u8 = 1;
+pub const ROOT_PARTITION_INDEX: u8 = 3;
+
+pub static JOURNALD_CONF: &str = "
+[Journal]
+Storage=volatile
+SystemMaxUse=16M
+";
+
+pub const BASE_PACKAGES: [&str; 6] = [
+    "base",
+    "grub",
+    "efibootmgr",
+    "intel-ucode",
+    "networkmanager",
+    "broadcom-wl",
+];

src/main.rs

@@ -1,4 +1,5 @@
 mod args;
+mod constants;
 mod error;
 mod initcpio;
 mod presets;
@@ -6,65 +7,71 @@ mod process;
 mod storage;
 mod tool;
 
-use crate::args::*;
-use crate::error::*;
-use crate::process::CommandExt;
-use crate::storage::*;
-use crate::tool::Tool;
+use args::Command;
 use byte_unit::Byte;
 use console::style;
 use dialoguer::{theme::ColorfulTheme, Select};
+use error::Error;
+use error::ErrorKind;
 use failure::{Fail, ResultExt};
 use log::{debug, error, info, log_enabled, Level, LevelFilter};
-use pretty_env_logger;
+use process::CommandExt;
 use std::collections::HashSet;
 use std::fs;
 use std::io::Write;
-use std::os::unix::{fs::PermissionsExt, process::CommandExt as UnixCommandExt};
+use std::os::unix::fs::PermissionsExt;
 use std::path::{Path, PathBuf};
 use std::process::{exit, Command as ProcessCommand};
 use std::thread;
 use std::time::Duration;
+use storage::EncryptedDevice;
+use storage::{BlockDevice, Filesystem, FilesystemType, LoopDevice};
 use structopt::StructOpt;
 use tempfile::tempdir;
+use tool::Tool;
 
-const BOOT_PARTITION_INDEX: u8 = 1;
-const ROOT_PARTITION_INDEX: u8 = 3;
+fn main() {
+    // Get struct of args using structopt
+    let app = args::App::from_args();
 
-static JOURNALD_CONF: &'static str = "
-[Journal]
-Storage=volatile
-SystemMaxUse=16M
-";
+    // Set up logging
+    let mut builder = pretty_env_logger::formatted_timed_builder();
+    let log_level = if app.verbose {
+        LevelFilter::Debug
+    } else {
+        LevelFilter::Info
+    };
+    builder.filter_level(log_level);
+    builder.init();
 
-fn mount<'a>(
-    mount_path: &Path,
-    boot_filesystem: &'a Filesystem,
-    root_filesystem: &'a Filesystem,
-) -> Result<MountStack<'a>, Error> {
-    let mut mount_stack = MountStack::new();
-    debug!(
-        "Root partition: {}",
-        root_filesystem.block().path().display()
-    );
+    // Match command from arguments and run relevant code
+    let result = match app.cmd {
+        Command::Create(command) => create(command),
+        Command::Chroot(command) => tool::chroot(command),
+        Command::Qemu(command) => tool::qemu(command),
+    };
 
-    info!("Mounting filesystems to {}", mount_path.display());
-    mount_stack
-        .mount(&root_filesystem, mount_path.into(), None)
-        .context(ErrorKind::Mounting)?;
-
-    let boot_point = mount_path.join("boot");
-    if !boot_point.exists() {
-        fs::create_dir(&boot_point).context(ErrorKind::CreateBoot)?;
+    // Check if command return an Error
+    // Print all causes to stderr if so
+    match result {
+        Ok(()) => {
+            exit(0);
+        }
+        Err(error) => {
+            error!("{}", error);
+            for cause in (&error as &dyn Fail).iter_causes() {
+                error!("Caused by: {}", cause);
+            }
+            exit(1);
+        }
     }
-
-    mount_stack
-        .mount(&boot_filesystem, boot_point, None)
-        .context(ErrorKind::Mounting)?;
-
-    Ok(mount_stack)
 }
 
+/// Remove swap entry from fstab and any commented lines
+/// Returns an owned String
+///
+/// # Arguments
+/// * `fstab` - A string slice holding the contents of the fstab file
 fn fix_fstab(fstab: &str) -> String {
     fstab
         .lines()
@@ -73,6 +80,7 @@ fn fix_fstab(fstab: &str) -> String {
         .join("\n")
 }
 
+/// Creates a file at the path provided, and mounts it to a loop device
 fn create_image(path: &Path, size: Byte, overwrite: bool) -> Result<LoopDevice, Error> {
     {
         let mut options = fs::OpenOptions::new();
@@ -92,11 +100,12 @@ fn create_image(path: &Path, size: Byte, overwrite: bool) -> Result<LoopDevice,
     LoopDevice::create(path)
 }
 
+/// Requests selection of block device (no device was given in the arguments)
 fn select_block_device(allow_non_removable: bool) -> Result<PathBuf, Error> {
-    let devices = get_storage_devices(allow_non_removable)?;
+    let devices = storage::get_storage_devices(allow_non_removable)?;
 
     if devices.is_empty() {
-        Err(ErrorKind::NoRemovableDevices)?
+        return Err(ErrorKind::NoRemovableDevices.into());
     }
 
     if allow_non_removable {
@@ -118,9 +127,10 @@ fn select_block_device(allow_non_removable: bool) -> Result<PathBuf, Error> {
     Ok(PathBuf::from("/dev").join(&devices[selection].name))
 }
 
-#[allow(clippy::cognitive_complexity)]
-fn create(command: CreateCommand) -> Result<(), Error> {
-    let presets = presets::Presets::load(&command.presets)?;
+/// Creates the installation
+#[allow(clippy::cognitive_complexity)] // TODO: Split steps into functions and remove this
+fn create(command: args::CreateCommand) -> Result<(), Error> {
+    let presets = presets::PresetsCollection::load(&command.presets)?;
 
     let sgdisk = Tool::find("sgdisk")?;
     let pacstrap = Tool::find("pacstrap")?;
@@ -185,10 +195,10 @@ fn create(command: CreateCommand) -> Result<(), Error> {
     thread::sleep(Duration::from_millis(1000));
 
     info!("Formatting filesystems");
-    let boot_partition = storage_device.get_partition(BOOT_PARTITION_INDEX)?;
+    let boot_partition = storage_device.get_partition(constants::BOOT_PARTITION_INDEX)?;
     let boot_filesystem = Filesystem::format(&boot_partition, FilesystemType::Vfat, &mkfat)?;
 
-    let root_partition_base = storage_device.get_partition(ROOT_PARTITION_INDEX)?;
+    let root_partition_base = storage_device.get_partition(constants::ROOT_PARTITION_INDEX)?;
     let encrypted_root = if let Some(cryptsetup) = &cryptsetup {
         info!("Encrypting the root filesystem");
         EncryptedDevice::prepare(&cryptsetup, &root_partition_base)?;
@@ -209,7 +219,7 @@ fn create(command: CreateCommand) -> Result<(), Error> {
 
     let root_filesystem = Filesystem::format(root_partition, FilesystemType::Ext4, &mkext4)?;
 
-    let mount_stack = mount(mount_point.path(), &boot_filesystem, &root_filesystem)?;
+    let mount_stack = tool::mount(mount_point.path(), &boot_filesystem, &root_filesystem)?;
 
     if log_enabled!(Level::Debug) {
         debug!("lsblk:");
@@ -223,17 +233,10 @@ fn create(command: CreateCommand) -> Result<(), Error> {
             .ok();
     }
 
-    let mut packages: HashSet<String> = [
-        "base",
-        "grub",
-        "efibootmgr",
-        "intel-ucode",
-        "networkmanager",
-        "broadcom-wl",
-    ]
-    .iter()
-    .map(|s| String::from(*s))
-    .collect();
+    let mut packages: HashSet<String> = constants::BASE_PACKAGES
+        .iter()
+        .map(|s| String::from(*s))
+        .collect();
 
     packages.extend(presets.packages);
 
@@ -292,7 +295,7 @@ fn create(command: CreateCommand) -> Result<(), Error> {
     info!("Configuring journald");
     fs::write(
         mount_point.path().join("etc/systemd/journald.conf"),
-        JOURNALD_CONF,
+        constants::JOURNALD_CONF,
     )
     .context(ErrorKind::PostInstallation)?;
 
@@ -378,123 +381,3 @@ fn create(command: CreateCommand) -> Result<(), Error> {
 
     Ok(())
 }
-
-fn chroot(command: ChrootCommand) -> Result<(), Error> {
-    let arch_chroot = Tool::find("arch-chroot")?;
-    let cryptsetup;
-
-    let loop_device: Option<LoopDevice>;
-    let storage_device =
-        match storage::StorageDevice::from_path(&command.block_device, command.allow_non_removable)
-        {
-            Ok(b) => b,
-            Err(_) => {
-                loop_device = Some(LoopDevice::create(&command.block_device)?);
-                storage::StorageDevice::from_path(
-                    loop_device.as_ref().unwrap().path(),
-                    command.allow_non_removable,
-                )?
-            }
-        };
-    let mount_point = tempdir().context(ErrorKind::TmpDirError)?;
-
-    let boot_partition = storage_device.get_partition(BOOT_PARTITION_INDEX)?;
-    let boot_filesystem = Filesystem::from_partition(&boot_partition, FilesystemType::Vfat);
-
-    let root_partition_base = storage_device.get_partition(ROOT_PARTITION_INDEX)?;
-    let encrypted_root = if is_encrypted_device(&root_partition_base)? {
-        cryptsetup = Some(Tool::find("cryptsetup")?);
-        Some(EncryptedDevice::open(
-            cryptsetup.as_ref().unwrap(),
-            &root_partition_base,
-            "alma_root".into(),
-        )?)
-    } else {
-        None
-    };
-
-    let root_partition = if let Some(e) = encrypted_root.as_ref() {
-        e as &dyn BlockDevice
-    } else {
-        &root_partition_base as &dyn BlockDevice
-    };
-    let root_filesystem = Filesystem::from_partition(root_partition, FilesystemType::Ext4);
-
-    let mount_stack = mount(mount_point.path(), &boot_filesystem, &root_filesystem)?;
-
-    arch_chroot
-        .execute()
-        .arg(mount_point.path())
-        .args(&command.command)
-        .run(ErrorKind::Interactive)?;
-
-    info!("Unmounting filesystems");
-    mount_stack.umount()?;
-
-    Ok(())
-}
-
-fn qemu(command: QemuCommand) -> Result<(), Error> {
-    let qemu = Tool::find("qemu-system-x86_64")?;
-
-    let mut run = qemu.execute();
-    run.args(&[
-        "-m",
-        "4G",
-        "-netdev",
-        "user,id=user.0",
-        "-device",
-        "virtio-net-pci,netdev=user.0",
-        "-device",
-        "qemu-xhci,id=xhci",
-        "-device",
-        "usb-tablet,bus=xhci.0",
-        "-drive",
-    ])
-    .arg(format!(
-        "file={},if=virtio,format=raw",
-        command.block_device.display()
-    ))
-    .args(command.args);
-
-    if PathBuf::from("/dev/kvm").exists() {
-        debug!("KVM is enabled");
-        run.args(&["-enable-kvm", "-cpu", "host"]);
-    }
-
-    let err = run.exec();
-
-    Err(err).context(ErrorKind::Qemu)?
-}
-
-fn main() {
-    let app = App::from_args();
-
-    let mut builder = pretty_env_logger::formatted_timed_builder();
-    let log_level = if app.verbose {
-        LevelFilter::Debug
-    } else {
-        LevelFilter::Info
-    };
-    builder.filter_level(log_level);
-    builder.init();
-
-    let result = match app.cmd {
-        Command::Create(command) => create(command),
-        Command::Chroot(command) => chroot(command),
-        Command::Qemu(command) => qemu(command),
-    };
-
-    match result {
-        Ok(()) => {
-            exit(0);
-        }
-        Err(error) => {
-            error!("{}", error);
-            for cause in (&error as &dyn Fail).iter_causes() {
-                error!("Caused by: {}", cause);
-            }
-            exit(1);
-        }
-    }
-}

src/presets.rs

@@ -5,10 +5,8 @@ use std::collections::HashSet;
 use std::env;
 use std::fs;
 use std::path::{Path, PathBuf};
-use toml;
 
 #[derive(Deserialize)]
-
 struct Preset {
     packages: Option<Vec<String>>,
     script: Option<String>,
@@ -24,12 +22,12 @@ impl Preset {
     }
 }
 
-pub struct Presets {
+pub struct PresetsCollection {
     pub packages: HashSet<String>,
     pub scripts: Vec<String>,
 }
 
-impl Presets {
+impl PresetsCollection {
     pub fn load(list: &[PathBuf]) -> Result<Self, Error> {
         let mut packages = HashSet::new();
         let mut scripts = Vec::new();
@@ -59,7 +57,7 @@ impl Presets {
             .collect();
 
         if !missing_envrionments.is_empty() {
-            Err(ErrorKind::MissingEnvironmentVariables(missing_envrionments))?
+            return Err(ErrorKind::MissingEnvironmentVariables(missing_envrionments).into());
         }
 
         Ok(Self { packages, scripts })

src/process.rs

@@ -1,4 +1,4 @@
-use super::error::*;
+use super::error::{Error, ErrorKind};
 use failure::{Fail, ResultExt};
 use log::error;
 use std::process::{Command, ExitStatus};

src/storage/crypt.rs

@@ -9,8 +9,8 @@ use std::io::Read;
 use std::marker::PhantomData;
 use std::path::{Path, PathBuf};
 
-static LUKS_MAGIC_1: &'static [u8] = &[0x4c, 0x55, 0x4b, 0x53, 0xba, 0xbe];
-static LUKS_MAGIC_2: &'static [u8] = &[0x53, 0x4b, 0x55, 0x4c, 0xba, 0xbe];
+static LUKS_MAGIC_1: &[u8] = &[0x4c, 0x55, 0x4b, 0x53, 0xba, 0xbe];
+static LUKS_MAGIC_2: &[u8] = &[0x53, 0x4b, 0x55, 0x4c, 0xba, 0xbe];
 
 #[derive(Debug)]
 pub struct EncryptedDevice<'t, 'o> {

src/storage/loop_device.rs

@@ -21,9 +21,7 @@ impl LoopDevice {
             .context(ErrorKind::Image)?;
 
         if !output.status.success() {
-            Err(ErrorKind::Losetup(
-                String::from_utf8(output.stderr).unwrap(),
-            ))?
+            return Err(ErrorKind::Losetup(String::from_utf8(output.stderr).unwrap()).into());
         }
 
         let path = PathBuf::from(String::from_utf8(output.stdout).unwrap().trim());

src/storage/markers.rs

@@ -1,5 +1,5 @@
 use std::path::Path;
-
+// Marker traits
 pub trait BlockDevice: std::fmt::Debug {
     fn path(&self) -> &Path;
 }

src/storage/mount_stack.rs

@@ -2,7 +2,6 @@ use super::Filesystem;
 use crate::error::{Error, ErrorKind};
 use failure::Fail;
 use log::{debug, warn};
-use nix;
 use nix::mount::{mount, umount, MsFlags};
 use std::marker::PhantomData;
 use std::path::PathBuf;
@@ -20,7 +19,6 @@ impl<'a> MountStack<'a> {
         }
     }
 
-    #[must_use]
     pub fn mount(
         &mut self,
         filesystem: &'a Filesystem,

src/storage/storage_device.rs

@@ -31,8 +31,11 @@ impl<'a> StorageDevice<'a> {
             path,
             origin: PhantomData,
         };
-        if !allow_non_removable && (!(_self.is_removable_device()? || _self.is_loop_device())) {
-            return Err(ErrorKind::DangerousDevice)?;
+
+        // If we only allow removable/loop devices, and the device is neither removable or a loop
+        // device then throw a DangerousDevice error
+        if !(allow_non_removable || _self.is_removable_device()? || _self.is_loop_device()) {
+            return Err(ErrorKind::DangerousDevice.into());
         }
 
         Ok(_self)

src/tool/chroot.rs

@@ -0,0 +1,70 @@
+use super::mount;
+use super::Tool;
+use crate::args;
+use crate::constants::{BOOT_PARTITION_INDEX, ROOT_PARTITION_INDEX};
+use crate::error::{Error, ErrorKind};
+use crate::process::CommandExt;
+use crate::storage;
+use crate::storage::{is_encrypted_device, EncryptedDevice};
+use crate::storage::{BlockDevice, Filesystem, FilesystemType, LoopDevice};
+use log::info;
+
+use failure::ResultExt;
+use tempfile::tempdir;
+
+/// Use arch-chroot to chroot to the given device
+/// Also handles encrypted root partitions (detected by checking for the LUKS magic header)
+pub fn chroot(command: args::ChrootCommand) -> Result<(), Error> {
+    let arch_chroot = Tool::find("arch-chroot")?;
+    let cryptsetup;
+
+    let loop_device: Option<LoopDevice>;
+    let storage_device =
+        match storage::StorageDevice::from_path(&command.block_device, command.allow_non_removable)
+        {
+            Ok(b) => b,
+            Err(_) => {
+                loop_device = Some(LoopDevice::create(&command.block_device)?);
+                storage::StorageDevice::from_path(
+                    loop_device.as_ref().unwrap().path(),
+                    command.allow_non_removable,
+                )?
+            }
+        };
+    let mount_point = tempdir().context(ErrorKind::TmpDirError)?;
+
+    let boot_partition = storage_device.get_partition(BOOT_PARTITION_INDEX)?;
+    let boot_filesystem = Filesystem::from_partition(&boot_partition, FilesystemType::Vfat);
+
+    let root_partition_base = storage_device.get_partition(ROOT_PARTITION_INDEX)?;
+    let encrypted_root = if is_encrypted_device(&root_partition_base)? {
+        cryptsetup = Some(Tool::find("cryptsetup")?);
+        Some(EncryptedDevice::open(
+            cryptsetup.as_ref().unwrap(),
+            &root_partition_base,
+            "alma_root".into(),
+        )?)
+    } else {
+        None
+    };
+
+    let root_partition = if let Some(e) = encrypted_root.as_ref() {
+        e as &dyn BlockDevice
+    } else {
+        &root_partition_base as &dyn BlockDevice
+    };
+    let root_filesystem = Filesystem::from_partition(root_partition, FilesystemType::Ext4);
+
+    let mount_stack = mount(mount_point.path(), &boot_filesystem, &root_filesystem)?;
+
+    arch_chroot
+        .execute()
+        .arg(mount_point.path())
+        .args(&command.command)
+        .run(ErrorKind::Interactive)?;
+
+    info!("Unmounting filesystems");
+    mount_stack.umount()?;
+
+    Ok(())
+}

src/tool/mod.rs

@@ -1,4 +1,12 @@
-use super::error::*;
+mod chroot;
+mod mount;
+mod qemu;
+
+pub use chroot::chroot;
+pub use mount::mount;
+pub use qemu::qemu;
+
+use crate::error::*;
 use failure::ResultExt;
 use std::path::PathBuf;
 use std::process::Command;

src/tool/mount.rs

@@ -0,0 +1,37 @@
+use crate::error::{Error, ErrorKind};
+use crate::storage::{Filesystem, MountStack};
+use failure::ResultExt;
+use log::{debug, info};
+use std::fs;
+use std::path::Path;
+
+/// Mounts root filesystem to given mount_path
+/// Mounts boot filesystem to mount_path/boot
+/// Note we mount with noatime to reduce disk writes by not recording file access times
+pub fn mount<'a>(
+    mount_path: &Path,
+    boot_filesystem: &'a Filesystem,
+    root_filesystem: &'a Filesystem,
+) -> Result<MountStack<'a>, Error> {
+    let mut mount_stack = MountStack::new();
+    debug!(
+        "Root partition: {}",
+        root_filesystem.block().path().display()
+    );
+
+    info!("Mounting filesystems to {}", mount_path.display());
+    mount_stack
+        .mount(&root_filesystem, mount_path.into(), None)
+        .context(ErrorKind::Mounting)?;
+
+    let boot_point = mount_path.join("boot");
+    if !boot_point.exists() {
+        fs::create_dir(&boot_point).context(ErrorKind::CreateBoot)?;
+    }
+
+    mount_stack
+        .mount(&boot_filesystem, boot_point, None)
+        .context(ErrorKind::Mounting)?;
+
+    Ok(mount_stack)
+}

src/tool/qemu.rs

@@ -0,0 +1,43 @@
+use super::Tool;
+use crate::args;
+use crate::error;
+use log::debug;
+
+use failure::ResultExt;
+use std::os::unix::process::CommandExt as UnixCommandExt;
+use std::path::PathBuf;
+
+/// Loads given block device in qemu
+/// Uses kvm if it is enabled
+pub fn qemu(command: args::QemuCommand) -> Result<(), error::Error> {
+    let qemu = Tool::find("qemu-system-x86_64")?;
+
+    let mut run = qemu.execute();
+    run.args(&[
+        "-m",
+        "4G",
+        "-netdev",
+        "user,id=user.0",
+        "-device",
+        "virtio-net-pci,netdev=user.0",
+        "-device",
+        "qemu-xhci,id=xhci",
+        "-device",
+        "usb-tablet,bus=xhci.0",
+        "-drive",
+    ])
+    .arg(format!(
+        "file={},if=virtio,format=raw",
+        command.block_device.display()
+    ))
+    .args(command.args);
+
+    if PathBuf::from("/dev/kvm").exists() {
+        debug!("KVM is enabled");
+        run.args(&["-enable-kvm", "-cpu", "host"]);
+    }
+
+    let err = run.exec();
+
+    Err(err).context(error::ErrorKind::Qemu)?
+}