Re: [pbs-devel] [PATCH v2 proxmox-backup 01/15] tools: add disks utility functions

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 8/30/21 13:14, Hannes Laimer wrote:
> adds:
>   - get_fs_uuid_by_disk_path: taken out of the already existing
>      get_fs_uuid function
>   - get_mount_point_by_uuid: returns the mount-point of a disk by a given
>      uuid, fails if the disk is not mounted
>   - get_fs_uuid_by_path: finds the uuid of the disk that the given path
>      is on, can fail if there is no uuid for the disk
>   - mount_by_uuid: mounts a disk identified by uuid to a given path
>   - unmount_by_mount_point: basically linux 'umount -l', justification
>      for the lazy option in code
> ---
>   src/tools/disks/mod.rs | 53 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index af7f7fe1..ec4d8742 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -1009,6 +1009,22 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
>           None => bail!("disk {:?} has no node in /dev", disk.syspath()),
>       };
>   
> +    get_fs_uuid_by_disk_path(&disk_path)
> +}
> +
> +/// Get the FS UUID of the disk that the given path is on.
> +pub fn get_fs_uuid_by_path(path: &Path) -> Result<String, Error> {
> +    let mut command = std::process::Command::new("df");
> +    command.args(&["--output=source"]);
> +    command.arg(path);
> +
> +    let output = crate::tools::run_command(command, None)?;

AFAICS, we already have that information in
DiskManage::find_mounted_device ?

would it not make sense to add the uuid information there?

this would save us a fork

> +
> +    get_fs_uuid_by_disk_path(Path::new(output.lines().skip(1).next().unwrap()))

although it seems unlikely to make a problem (for now), please do
not use unwrap in the middle of code. when df sometime decides to change
output (or some other unpredictable thing happens) this will panic, and
depending where that code is executed, it can have bad effects on the
rest of the code (e.g. when holding a mutex)

instead use e.g. '.ok_or_else' or use something like
if let Some(foo) = output....next() { ... } else { bail!(...) }

> +}
> +
> +/// Get the FS UUID of a disk with a given disk path.
> +pub fn get_fs_uuid_by_disk_path(disk_path: &Path) -> Result<String, Error> {
>       let mut command = std::process::Command::new("blkid");
>       command.args(&["-o", "export"]);
>       command.arg(disk_path);
> @@ -1023,3 +1039,40 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
>   
>       bail!("get_fs_uuid failed - missing UUID");
>   }
> +
> +/// Get mount point by UUID
> +pub fn get_mount_point_by_uuid(uuid: &str) -> Result<String, Error> {
> +    let mut command = std::process::Command::new("findmnt");
> +    command.args(&["-rn", "-oTARGET", "-S"]);
> +    command.arg(&format!("UUID={}", uuid));
> +
> +    let output = crate::tools::run_command(command, None)?;
> +
> +    if !output.is_empty() {
> +        return Ok(String::from(output.trim()));
> +    }
> +
> +    bail!("get_mount_point_by_uuid failed - device with uuid: {} is not mounted", uuid);
> +}
> +
> +/// Mount a disk by its UUID and the mount point.
> +pub fn mount_by_uuid(uuid: &str, mount_point: &Path) -> Result<(), Error> {
> +    let mut command = std::process::Command::new("mount");
> +    command.args(&[&format!("UUID={}", uuid)]);
> +    command.arg(mount_point);
> +
> +    crate::tools::run_command(command, None)?;
> +    Ok(())
> +}
> +
> +/// Unmount a disk by its mount point.
> +pub fn unmount_by_mount_point(mount_point: &Path) -> Result<(), Error> {
> +    let mut command = std::process::Command::new("umount");
> +    // Datastores are only unmounted after a check for running jobs, '-l' needed due to some weird
> +    // bug where some tokio-threads keep the .lock-file open (sometimes)
> +    command.args(&["-l"]);
> +    command.arg(mount_point);
> +
> +    crate::tools::run_command(command, None)?;
> +    Ok(())
> +}
>