3
\$\begingroup\$

I'm writing a system daemon in Rust that may be started by the user manually. Only one instance of the daemon should be running at a time so I have created a pidfile locking mechanism to prevent multiple instances. This is my first time doing any kind of unix style file locking and I would appreciate another pair of eyes to check for anything I may have missed.
Style pointers would also be great as I am new to Rust.

use nix::fcntl::{flock, FlockArg};
use nix::sys::signal::kill;
use nix::unistd::Pid;
use std::fs::File;
use std::fs::OpenOptions;
use std::io;
use std::io::{Error, ErrorKind, Read, Seek, SeekFrom, Write};
use std::os::unix::io::AsRawFd;
use std::path;
use std::process;

fn write_pid(mut file: &File) -> Result<(), io::Error> {
    let id = process::id().to_string() + "\n";
    file.write_all(id.as_bytes())?;
    Ok(())
}

fn lock_pidfile(file: &File, pidfile: &path::Path) -> Result<(), io::Error> {
    flock(file.as_raw_fd(), FlockArg::LockExclusiveNonblock).map_err(|_| {
        Error::new(
            ErrorKind::Other,
            format!("Failed to lock pidfile: {}", pidfile.display()),
        )
    })
}

pub fn exclusive(pidfile: &path::Path) -> Result<bool, io::Error> {
    let pf = OpenOptions::new()
        .write(true)
        .create_new(true)
        .open(pidfile);

    match pf {
        Ok(file) => {
            lock_pidfile(&file, pidfile)?;
            write_pid(&file)?;
            Ok(true)
        }
        Err(err) => {
            match err.kind() {
                ErrorKind::AlreadyExists => {
                    let mut file = OpenOptions::new().read(true).write(true).open(pidfile)?;
                    lock_pidfile(&file, pidfile)?;
                    let mut id_str = String::new();
                    file.read_to_string(&mut id_str)?;
                    let id: u32 = id_str.trim().parse().map_err(|_| {
                        Error::new(
                            ErrorKind::Other,
                            format!("Failed to parse pidfile: {}", pidfile.display()),
                        )
                    })?;
                    // Kill None just checks if process exists.
                    // Same as kill(pid, 0); in C
                    if kill(Pid::from_raw(id as i32), None).is_ok() {
                        Ok(false)
                    } else {
                        file.seek(SeekFrom::Start(0))?;
                        write_pid(&file)?;
                        Ok(true)
                    }
                }
                _ => Err(err),
            }
        }
    }
}
```
\$\endgroup\$
  • \$\begingroup\$ use std::io; use std::io::{Error, ErrorKind, Read, Seek, SeekFrom, Write}; can be rewritten as use std::io::{self, Error, ...}. Also collapse fs::File and fs::OpenOptions. \$\endgroup\$ – hellow May 23 at 12:49
  • \$\begingroup\$ @hellow Thanks, did not think to use self less use lines make it a lot cleaner \$\endgroup\$ – Fyrn May 23 at 15:08
0
\$\begingroup\$

I don't know much about file locking, but I can guide you in Rust.

I'd suggest changing your write_pid to use writeln!. I think you can simplify it to this.

fn write_pid(mut file: &File) -> Result<(), io::Error> {
    writeln!(file, "{}", process::id())
}

I'd also prefer to reduce the nesting in exclusive, perhaps something like this:

pub fn exclusive(pidfile: &path::Path) -> Result<bool, io::Error> {
    let pf = OpenOptions::new()
        .write(true)
        .create_new(true)
        .open(pidfile);

    match pf {
        Ok(file) => {
            lock_pidfile(&file, pidfile)?;
            write_pid(&file)?;
            return Ok(true);
        }
        Err(ref e) if e.kind() == ErrorKind::AlreadyExists => {}
        Err(e) => return Err(e),
    }

    let mut file = OpenOptions::new().read(true).write(true).open(pidfile)?;
    lock_pidfile(&file, pidfile)?;
    let mut id_str = String::new();
    file.read_to_string(&mut id_str)?;
    let id: u32 = id_str.trim().parse().map_err(|_| {
        Error::new(
            ErrorKind::Other,
            format!("Failed to parse pidfile: {}", pidfile.display()),
        )
    })?;
    // Kill None just checks if process exists.
    // Same as kill(pid, 0); in C
    if kill(Pid::from_raw(id as i32), None).is_ok() {
        Ok(false)
    } else {
        file.seek(SeekFrom::Start(0))?;
        write_pid(&file)?;
        Ok(true)
    }
}
\$\endgroup\$
  • \$\begingroup\$ I've never seen the syntax you use here: Err(e) if e.kind() == ErrorKind::AlreadyExists => {], does the part at the end mean continue on? {] <-- this part edit: ah you probably meant {} as in do nothing. \$\endgroup\$ – Fyrn May 24 at 10:20
  • \$\begingroup\$ I according to the compiler this also needs ref: Err(ref e) because it can't move into the pattern guard \$\endgroup\$ – Fyrn May 24 at 10:46
  • \$\begingroup\$ I'm going to accept your answer because it makes the code a lot cleaner. I ended up doing my own research and found one more correctness bug: I was not checking for the case where kill fails because of a lack of permissions. I also realized that pid might not be unique, but because I want the code to be cross platform I'm going to hold off implementing process start time checking, as that is quite different between linux and macOS. \$\endgroup\$ – Fyrn May 24 at 11:01
  • \$\begingroup\$ Yep, sorry, I didn't run the code. I meant {} for an empty block. \$\endgroup\$ – JayDepp May 24 at 20:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.