Compiler warns for unhandled results

help
1

Hello everybody,

this is my first time post, so first of all "Hello, world!".

These days, I am learning rust, and currently, I am working on a unix keylogger, hooked between the keyboard and the unix system (in my case Linux).

The keylogger will run as a background service, which forces me to manage so called PID files. Since I am learning rust, I try write clean code, which includes effort to get rid of all warnings.

So, that's the problem. I have this snippet:

///
/// Reads the content of the PID_FILE.
/// None, if the file does not exist, or if reading fails.
/// otherwise Some(pid)
///
fn read_pid_file(f: &str) -> Option<u32> {
    let mut pid = None;

    if std::path::Path::new(f).exists() {
        // read file and fill pid, if file contains value
        let pidf = std::fs::OpenOptions::new()
            .read(true)
            .open(f);

        if pidf.is_ok() {
            let mut buf = String::new();
            pidf.unwrap().read_to_string(&mut buf);
            pid = Some(buf.parse::<u32>().unwrap());
        }
    }

    pid
}

The warning is the following:

warning: unused `std::result::Result` that must be used
  --> src/os/process.rs:56:13
     |
56 |             pidf.unwrap().read_to_string(&mut buf);
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    = note: this `Result` may be an `Err` variant, which should be handled

As you can see, the method will return "None" if something goes wrong, otherwise it will return Some().
Reading the file will return a result, which needs to be unwrapped. I am checking this with the preceding if pidf.is_ok(). But still, the compiler generates warnings to handle the (im)possible case of an error.

How can I get rid of the warning? Is there a better way to write my code? I would like to have a short & clean solution.

Thanks for any input, remarks or different. If I violate any rust paradigms please let me know. I would love to enhance my rust knowledge, which requires feedback.

Thanks for any response.

Best regards
Max

2 
let _ = pidf.unwrap().read_to_string(&mut buf);

will immediately discard the Result and remove the warning.

3

The read_to_string method returns a Result, which you are not handling.

You would need to add another unwrap() after read_to_string.

Note though that you should probably move the error handling one layer up and handle the errors there. unwrap() is sometimes ok but in this case not a good solution.

Eg:

fn read_pid_file(f: &str) -> Result<Option<u32>, std::io::Error> {
    let opt = if std::path::Path::new(f).exists() {
        // read file and fill pid, if file contains value
        let pidf = std::fs::OpenOptions::new()
            .read(true)
            .open(f)?;

            let mut buf = String::new();
            pidf.read_to_string(&mut buf)?;
            buf.parse::<u32>().ok()
    } else {
        None
    };
    Ok(opt)
}

Note the ? operator to propagate the error upwards.

Also: this still discards a parsing error by using .ok() to turn a Result into a Option.
You might handle this differently by using a custom error enum like

enum PidError {
  Io(std::io::Error),
  Parse(ParseError),
}
2 Likes
4

Thanks, this is a good hint!

5

Thanks for your long answer. One question: Is it really necessary, to return Result?
In my case, the read_pid_file() might fail, which then returns a None. And in case of a none, some other algorithms will ensure, that the correct PID will be determined.

I'll try to check on which extend your solution will help me in my case. I'll probably include parts of it, but not all. Especially the extra enum seems to be a bit too much for a single function, doesn't it?

6

To have robust code, yes.
Eg anunwraps could easily panic with eg a permission error (quite common for daemons) on reading, which you really want to handle gracefully for a with a good error message for the user instead of a cryptic Permission denied panic. Or if you were to convert to an option you would just get None back and would produce a permission denied error when trying to write the new file.

Discarding the parse error and just using std::io::Error is probably fine in this case though.

7

Well, in my original code, I was just unwrapping under the condition

    if pidf.is_ok() {
        pidf.unwrap()
        [...]
    }

Am I wrong? Is there still the option that it can panic?

I have included your solution now, but this is still a question I have.

Thanks for your patience.

8

True, the original code would not panic for a permission error but would still produce a None, leading to a permission error when writing the new PID file. It would still panick with a cryptic error if the parsing failed.

Silently discarding errors is very rarely what you want. They should (almost) always either be handled explicitly or forwarded to the caller.

panicking by unwrap or expect() is of course a way to handle a error and would probably be a better solution here than your original if pidf.is_ok(), because it doesn't swallow an error without dealing with it.

Errors while dealing with a PID file is a pretty common error condition for daemons.
Panicking should usually only happen for unexpected or unrecoverable errors, and should be handled more gracefully with eg a good error message printed.

Like Startup error: could not read PID at /path/to/pid: permission denied

2 Likes
9

There are three possible sources of failure in read_pid_file:

  1. You might fail to open the file, for example because it doesn't exist. (You think you're checking for this but the check is insufficient; see below)
  2. You might successfully open the file, but fail to read from it, for example because it doesn't contain valid UTF-8.
  3. You might successfully open the file and read from it, but the data read can't be parsed into a u32.

You've tried to account for 1 and 3 but you're ignoring 2.

Your original code only panics when the parse fails and returns None for all other failures. Here's a nice concise way to write that:

fn read_pid_file(f: &str) -> Option<u32> {
    std::fs::read_to_string(f)
        .map(|buf| buf.parse::<u32>().unwrap())
        .ok()
}

(I didn't know std::fs::read_to_string existed until I searched the docs for read_to_string just now, but since it's there, we might as well use it.)

This has different behavior than your original function in two ways. In the original, because the return value of read_to_string is ignored, you might read part of the file and then encounter an error. When this happens, the part that was read remains in the buffer. So if the part that was read before the error happened is parseable as a u32, the original function just parses and returns it with no indication of any error happening. This could mean that you read 10 instead of 109 which is obviously completely wrong. std::fs::read_to_string will return an Err(io::Error) in this case, which ok() will turn into None.

You should not check whether the file exists with Path::exists before trying to open it. This subjects your code to a race condition where the file may be deleted in between checking it and trying to open it. If you like what @theduke's code does (returning Ok(None) when the file does not exist or cannot be parsed, and Err for other errors), you should go ahead and try to open the file, and handle std::io::ErrorKind::NotFound explicitly. Here's one way to do it:

fn read_pid_file(f: &str) -> Result<Option<u32>, std::io::Error> {
    std::fs::read_to_string(f)
        .map(|buf| buf.parse::<u32>().ok()) // parse the String when Ok
        .or_else(|e| {                      // transform NotFound into None when Err
            if e.kind() == std::io::ErrorKind::NotFound {
                Ok(None)
            } else {
                Err(e)
            }
        })
}
4 Likes
10

Thanks for giving this input. As I said, I am new to rust, and definitely have to learn a lot. Especially the Result and Option patterns are quite unusual for beginners. That's where I am still struggling a bit.

11

Also, thank you for your time. This seems to be a stable solution. I've implemented it. The tests are passing. Great!

12

This won't panic (because is_ok() returned true) but it's not the most idiomatic way of getting the value inside your Result.

You'd normally use pattern matching, so either with an explicit match:

match pidf {
  Ok(f) => /* do something with the file handle */,
  Err(e) => /* handle the error */,
}

Or if you only care about the happy case you can use if let:

if let Ok(f) = pidf {
  // do something with the file handle
}
1 Like
13

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.