From 48f2fd4586c022a6e298a265365f87f05fc40f4a Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Thu, 19 Mar 2020 15:55:10 -0400 Subject: [PATCH 1/3] blockdev: allow mounting partitions by label Instead of a `mount_boot` function, have a more generic `mount_partition_by_label` function which takes the label as a parameter. While we're here, make it even more generic by accepting mount flags so that if a caller just wants to mount read-only, they can. --- src/blockdev.rs | 40 +++++++++++++++++++++------------------- src/install.rs | 3 ++- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/blockdev.rs b/src/blockdev.rs index 111bd1280..07b2a7d7d 100644 --- a/src/blockdev.rs +++ b/src/blockdev.rs @@ -30,28 +30,36 @@ use tempfile; use crate::errors::*; -pub fn mount_boot(device: &str) -> Result { +pub fn mount_partition_by_label(device: &str, label: &str, flags: mount::MsFlags) -> Result { // get partition list let partitions = get_partitions(device)?; if partitions.is_empty() { bail!("couldn't find any partitions on {}", device); } - // find the boot partition - let boot_partitions = partitions + // find the partition with the matching label + let matching_partitions = partitions .iter() - .filter(|d| d.label.as_ref().unwrap_or(&"".to_string()) == "boot") + .filter(|d| d.label.as_ref().unwrap_or(&"".to_string()) == label) .collect::>(); - let dev = match boot_partitions.len() { - 0 => bail!("couldn't find boot device for {}", device), - 1 => boot_partitions[0], - _ => bail!("found multiple devices on {} with label \"boot\"", device), + let dev = match matching_partitions.len() { + 0 => bail!("couldn't find {} device for {}", label, device), + 1 => matching_partitions[0], + _ => bail!( + "found multiple devices on {} with label \"{}\"", + device, + label + ), }; // mount it match &dev.fstype { - Some(fstype) => Mount::try_mount(&dev.path, &fstype), - None => Err(format!("couldn't get filesystem type of boot device for {}", device).into()), + Some(fstype) => Mount::try_mount(&dev.path, &fstype, flags), + None => Err(format!( + "couldn't get filesystem type of {} device for {}", + label, device + ) + .into()), } } @@ -126,7 +134,7 @@ pub struct Mount { } impl Mount { - fn try_mount(device: &str, fstype: &str) -> Result { + fn try_mount(device: &str, fstype: &str, flags: mount::MsFlags) -> Result { let tempdir = tempfile::Builder::new() .prefix("coreos-installer-") .tempdir() @@ -135,14 +143,8 @@ impl Mount { // the partition contents if umount failed let mountpoint = tempdir.into_path(); - mount::mount::( - Some(device), - &mountpoint, - Some(fstype), - mount::MsFlags::empty(), - None, - ) - .chain_err(|| format!("mounting device {} on {}", device, mountpoint.display()))?; + mount::mount::(Some(device), &mountpoint, Some(fstype), flags, None) + .chain_err(|| format!("mounting device {} on {}", device, mountpoint.display()))?; Ok(Mount { device: device.to_string(), diff --git a/src/install.rs b/src/install.rs index 20f4537bb..6899402b2 100644 --- a/src/install.rs +++ b/src/install.rs @@ -13,6 +13,7 @@ // limitations under the License. use error_chain::{bail, ChainedError}; +use nix::mount; use std::fs::{create_dir_all, read_dir, File, OpenOptions}; use std::io::{copy, Read, Seek, SeekFrom, Write}; use std::os::unix::fs::FileTypeExt; @@ -111,7 +112,7 @@ fn write_disk( // postprocess if ignition.is_some() || config.firstboot_kargs.is_some() || config.platform.is_some() { - let mount = mount_boot(&config.device)?; + let mount = mount_partition_by_label(&config.device, "boot", mount::MsFlags::empty())?; if let Some(ignition) = ignition { write_ignition(mount.mountpoint(), ignition)?; } From edc36ea1bd127f318c98cd6a399175d05bd7cf85 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Thu, 19 Mar 2020 16:00:39 -0400 Subject: [PATCH 2/3] blockdev: put ioctls in submodule The `ioctl!` macros always declare their functions as pub, so let's put it in a submodule to keep it contained. It also makes it really obvious in the rest of the code when we're calling an ioctl by having to prefix it with `ioctl::`. Another minor bonus is that we can slap on `allow(clippy::missing_safety_doc)` on the whole block instead of individually. (Maybe that lint might need tweaking to ignore unsafe functions derived from macros?). --- src/blockdev.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/blockdev.rs b/src/blockdev.rs index 07b2a7d7d..12b76d531 100644 --- a/src/blockdev.rs +++ b/src/blockdev.rs @@ -13,8 +13,7 @@ // limitations under the License. use error_chain::bail; -use nix::errno::Errno; -use nix::{self, ioctl_none, ioctl_read_bad, mount, request_code_none}; +use nix::{errno::Errno, mount}; use regex::Regex; use std::collections::HashMap; use std::convert::TryInto; @@ -186,7 +185,7 @@ pub fn reread_partition_table(file: &mut File) -> Result<()> { // Reread sometimes fails inexplicably. Retry several times before // giving up. for retries in (0..20).rev() { - let result = unsafe { blkrrpart(fd) }; + let result = unsafe { ioctl::blkrrpart(fd) }; match result { Ok(_) => break, Err(err) => { @@ -214,7 +213,7 @@ pub fn reread_partition_table(file: &mut File) -> Result<()> { pub fn get_sector_size(file: &mut File) -> Result { let fd = file.as_raw_fd(); let mut size: c_int = 0; - match unsafe { blksszget(fd, &mut size) } { + match unsafe { ioctl::blksszget(fd, &mut size) } { Ok(_) => { let size_u32: u32 = size .try_into() @@ -226,8 +225,13 @@ pub fn get_sector_size(file: &mut File) -> Result { } // create unsafe ioctl wrappers -ioctl_none!(blkrrpart, 0x12, 95); -ioctl_read_bad!(blksszget, request_code_none!(0x12, 104), c_int); +#[allow(clippy::missing_safety_doc)] +mod ioctl { + use super::c_int; + use nix::{ioctl_none, ioctl_read_bad, request_code_none}; + ioctl_none!(blkrrpart, 0x12, 95); + ioctl_read_bad!(blksszget, request_code_none!(0x12, 104), c_int); +} pub fn udev_settle() -> Result<()> { // "udevadm settle" silently no-ops if the udev socket is missing, and From d1120d9962f537b07c3efb2f128c7285342f57a4 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Thu, 19 Mar 2020 16:13:04 -0400 Subject: [PATCH 3/3] blockdev: drop unnecessary mutable reference --- src/blockdev.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blockdev.rs b/src/blockdev.rs index 12b76d531..67056efc6 100644 --- a/src/blockdev.rs +++ b/src/blockdev.rs @@ -210,7 +210,7 @@ pub fn reread_partition_table(file: &mut File) -> Result<()> { } /// Get the logical sector size of a block device. -pub fn get_sector_size(file: &mut File) -> Result { +pub fn get_sector_size(file: &File) -> Result { let fd = file.as_raw_fd(); let mut size: c_int = 0; match unsafe { ioctl::blksszget(fd, &mut size) } {