Make cached bundle files readonly

[alexander-bauer]

• After moving new cache files to their final destination, set their
  permissions to have readonly set true.
• This uses fs::set_permissions rather than the suggested
  ``fs::File::set_permissions, because the latter would require the final file to be open. We cannot change the permissions on the tempfile in this way, because mkstemp::TempFile` doesn't have the same
  `set_permissions` method.

Close #14

[pkgw]

Looks great, thanks! Just a couple of requests:

1. In the error cases, can you use the chain_err() function on the resulting Err structs to provide some more contextual information for the errors? There are a lot of places in the codebase that didn't do this because it took me a while to understand the error-chaining principle, but I'm trying to be better about it. I believe the code would be something like:

Err(e.into()).chain_err(|| format!("could not read permissions of cache file {}", final_path))

In case of an error, the resulting output would then look like:

error: could not read permissions of cache file /blah/blah
caused by: NFS error

Or whatever. Without the chaining the user just gets "NFS error" (or whatever) which is harder to interpret.

2. It would be better if we could use a set_permissions type function on the file handle itself, since it's faster and more secure to refer to files via their handles, rather than their paths, on Unix systems. If mkstemp doesn't support it, so be it. But, can you add a sentence to the comment above the code block saying "It would be better to change the permissions using the file handle directly, but this is not currently supported by mkstemp-rs."?

Thanks.

[alexander-bauer]

@pkgw I've worked on this, but I'm a bit hung up on the error chaining. (This is my first meaningful foray into Rust -- I'm somewhat out at sea.) In other places in the codebase where I see chain_err, the return type of the enclosing function is of Result, whereas in path_for_name, the type is of OpenResult.

I can't seem to turn the result of a chain_err into an OpenResult, and it looks like that's because chain_err doesn't produce the expected type. Here's what I'm seeing, resulting from

        let mut perms = match fs::metadata(&final_path) {
                Ok(p) => p,
                Err(e) => {
                    return OpenResult::Err(
                        Err(e.into()).chain_err(
                            || format!("could not read metadata of cache file {}",
                                       final_path.display())
                    ));
                }
            }
            .permissions();

Compiling tectonic v0.1.6-dev (file:///home/sasha/dev/tectonic)
error[E0308]: mismatched types
   --> src/io/local_cache.rs:345:25
    |
345 |                           Err(e.into()).chain_err(
    |  _________________________^ starting here...
346 | |                             || format!("could not read metadata of cache file {}",
347 | |                                        final_path.display())
348 | |                     ));
    | |_____________________^ ...ending here: expected struct `errors::Error`, found enum `std::result::Result`
    |
    = note: expected type `errors::Error`
               found type `std::result::Result<_, errors::Error>`
    = help: here are some functions which might fulfill your needs:
            - .unwrap()
            - .unwrap_err()
            - .unwrap_or_default()

Can you give some guidance?

Also, it looks like mkstemp has the file handle in a private field, and there's no way for us to get to it. I've added a note to this effect, but am stuck on the error handling.

[pkgw]

First of all, thanks for taking the plunge with working with Rust! I think it's a great language, but there is definitely a learning curve.

I think that I just gave you the wrong advice at first. Does it work if you try Err(e.into().chain_err(|| ...)) ? That is, moving the chain_err call inside the Err() constructor.

If that doesn't work, let's just forget it — skip the chain_err stuff, and we'll get this PR merged.

[alexander-bauer]

I've been wanting to take some time to learn Rust for a while, but hadn't found a reason, so this project is really doing me some good!

That doesn't work either, and gives me

error: the type of this value must be known in this context
   --> src/io/local_cache.rs:314:50
    |
314 |               Err(e) => return OpenResult::Err(Err(e.into().chain_err(
    |  __________________________________________________^ starting here...
315 | |                     || format!("could not get file metadata {}", final_path.display())))),
    | |_______________________________________________________________________________________^ ...ending here

error[E0308]: mismatched types
   --> src/io/local_cache.rs:314:46
    |
314 |               Err(e) => return OpenResult::Err(Err(e.into().chain_err(
    |  ______________________________________________^ starting here...
315 | |                     || format!("could not get file metadata {}", final_path.display())))),
    | |________________________________________________________________________________________^ ...ending here: expected struct `errors::Error`, found enum `std::result::Result`
    |
    = note: expected type `errors::Error`
               found type `std::result::Result<_, _>`
    = help: here are some functions which might fulfill your needs:
            - .unwrap()
            - .unwrap_err()
            - .unwrap_or_default()

error: aborting due to 2 previous errors

error: Could not compile `tectonic`.

It doesn't look like chain_err is used anywhere else in the file, so if there's an effort to go through and convert everything, these should get caught up there. Are there any other obvious things I can try before giving up on it, though?

[pkgw]

The message "the type of this value must be known in this context" makes me think that code might actually be correct, but that Rust's type inference needs help. This can usually be accomplished by using a temporary variable with an explicitly specified type ... but that's just getting ugly. I'll merge now and maybe worry about this later.

Some of these operations might be easier if I got rid of the OpenResult type ... I've been thinking that maybe Result<Option<T>> would make life easier.