String errors

[eastern-grey]

get_error() in ceph.rs tries to convert the error into a string:

    unsafe {
        strerror_r(n, buf.as_mut_ptr() as *mut c_char, buf.len());
    }

Because ceph.rs is returning negative error codes for things like ENOENT, this means operations like trying to delete a missing object end up returning "Unknown error: -2". If n is changed to -n, a more helpful "No such file or directory" is returned instead. I haven't tested this works across all the API surface, as I'm only using rados_object_remove() at the moment.

While it's not something you can change in 2.x without breaking things, it would be great if a future release returned an error that is more easily inspected by code - at the moment fragile string comparisons are required to distinguish between an idempotent delete and something going wrong.

[cholcombe973]

I'm open to ideas about how we can return better error code to the library callers. I thought converting the errors to a string would be nice but I see your point about string comparisons if you're trying to handle a particular error case. Possibly returning errno the same way nix does could be useful. If there's a better way to do it we should do that and then bump the crate major version. PR contributions are of course welcome :)

[eastern-grey]

I'm still quite new to rust, so I may be missing something, but what about making use of io::Error::from_raw_os_error? Ie, instead of

        unsafe {
            let ret_code = rados_remove(self.ioctx, object_name_str.as_ptr() as *const c_char);
            if ret_code < 0 {
                return Err(RadosError::new(try!(get_error(ret_code as i32))));
            }
        }

One could write

        let ret_code = unsafe { rados_remove(self.ioctx, object_name_str.as_ptr() as *const c_char) };
        if ret_code < 0 {
            return Err(std::io::Error::from_raw_os_error(-ret_code).into());
        }

That would avoid the unsafe get_error, and allow easy introspection, like err.kind() == io::ErrorKind::NotFound.

You could go one further and allow creation of RadosError objects from a return value:

impl From<c_int> for RadosError {
    fn from(retval: c_int) -> RadosError {
        RadosError::IoError(std::io::Error::from_raw_os_error(-retval))
    }
}

Then the original code would be

        let ret_code = unsafe { rados_remove(self.ioctx, object_name_str.as_ptr() as *const c_char) };
        if ret_code < 0 {
            return Err(RadosError::from(ret_code));
        }

or even

        let ret_code = unsafe { rados_remove(self.ioctx, object_name_str.as_ptr() as *const c_char) };
        if ret_code < 0 {
            return Err(ret_code.into())
        }

I don't have enough experience with rados to know if the whole API returns negative error codes however.

[cholcombe973]

Yeah I like that. I was thinking of using Errno so you could pattern match against it. Errno from nix is a convenience built on top of the libc error codes. Your approach would also work. I'm kinda leaning towards using the nix Errno instead of std:io::Error. The only reason being that the std::io::ErrorKind isn't exhaustive and would miss a bunch of error return codes. The nix one i believe has all of them. The code should be nearly identical.

[eastern-grey]

Sounds good :-)

[cholcombe973]

Would you want to build that or should i? I don't have a lot of spare time to work on this but I could try. I don't think the change should take all that long.

[eastern-grey]

Unfortunately I'm busy for the next few weeks, but I might be able to help out after that if it remains an issue.