Do we have a guideline for handling error?

[rdzhou]

Hi,
I'm trying to contribute some code but get stuck in error handling.

One example I found for returning error is below. However, the progname and err are both None, which seems not very helpful.

mesabox/src/posix/chmod/mod.rs

Lines 161 to 165 in e906540

	Err(MesaError {
	exitcode: exitcode,
	progname: None,
	err: None,
	})

Another example I found is below. This line is quite difficult to understand for new contributors.

mesabox/src/networking/ping/mod.rs

Line 279 in e906540

None => Err(failure::err_msg(format!("invalid hostname: {}", hostname_os.to_string_lossy())).compat())?,

Could you give some guildeline for error handling in mesabox?

[Arcterus]

There’s no guideline per se at the moment, but generally what you want to do is implement a type that derives Fail (such as ChmodError) and then map errors from the standard library or outside libraries to that type. I think cat is a fairly good example (IIRC). Both of the examples you found should be edge cases.

[rdzhou]

I see. Thank you for your reply. I can understand now. I think a simple tutorial will be quite helpful for new contributors.

[mssun]

I'll leave this issue open. When a consistent error handling method and guideline is ready, we can close this issue then.

[Arcterus]

I mean the method is generally pretty consistent (I don’t think it’s possible to make the setup fully general as the utilities have widely different errors) except for a few strange situations (or laziness). I agree that we need some sort of guide though.

[mssun]

derives Fail (such as ChmodError) and then map errors from the standard library or outside libraries to that type

Using failure is ok for me.

For consistency, I found that cat will return CatResult<T> (type CatResult<T> = ::std::result::Result<T, CatError>) but head will return super::Result<T> (pub type Result<T> = StdResult<T, MesaError>;). Although it can be implicit converted, it still very confusing when first reading the code.

Another example is where to handle (print) error message:

chmod use display_err! to print out error message.

mesabox/src/posix/chmod/mod.rs

Line 238 in 1b32bde

display_err!(self.stderr, "could not change permissions of directory '{}'", file.display())?;

head uses display-msg! to print out error message.

mesabox/src/posix/head/mod.rs

Lines 147 to 151 in 1b32bde

	if let Err(mut e) = res {
	display_msg!(err_stream, "{}", e)?;
	e.err = None;
	result = Err(e);
	}

But cat returns CatError and do not print any error messages.

[Arcterus]

Everything using display_err!() should switch to display_msg!() as we are getting rid of the error: prefix.

cat also prints errors (on phone so I can’t link). If the utilities didn’t, they would have to fail the moment they encounter an invalid file.

head uses super::Result because it doesn’t customize error messages. What specifically is confusing?

[mssun]

Ok, understand. I found the display_msg statement in cat. Thank you!

You mentioned about using display_msg!() to display error message. When should I use it? Should I display the error message when encountering it or just return to the caller and let it to handle/print it?

For this one brought up by @rdzhou :

mesabox/src/posix/chmod/mod.rs

Lines 156 to 166 in e906540

	let exitcode = chmoder.chmod(matches.values_of_os("FILES").unwrap())?;
	if exitcode == 0 {
	Ok(())
	} else {
	Err(MesaError {
	exitcode: exitcode,
	progname: None,
	err: None,
	})
	}

can be simplified, because the exitcode does nothing (the chmod function will return if it has error):

chmoder.chmod(matches.values_of_os("FILES").unwrap())?;

Also, this are more confusing:

mesabox/src/posix/chmod/mod.rs

Lines 210 to 224 in e906540

	fn chmod<'b>(&mut self, files: OsValues<'b>) -> Result<i32> {
	let mut r = 0;
	for filename in files {
	let file = util::actual_path(&self.current_dir, filename);
	r &= if file.is_dir() && self.recursive {
	self.chmod_dir(&file)
	} else {
	self.chmod_file(&file)
	}?;
	}
	Ok(r)
	}

Here, this returns error code or err type? DoesOk(1) mean there is an error? Seems that the code mixes traditional C error handling (by returning error code) with Rust's error (by returning Result) handling. In addition, for this specific function, is r always 0 (since you are using &=)?

[Arcterus]

It should be |=, so there must have been a typo :/

I’d need to spend some time to review but IIRC chmod returns Err() on fatal errors whereas Ok(1) indicates there was an issue but it was able to keep going.

I’m still not entirely decided about what to do with the last error that gets returned from execute() (which is automatically printed). On one hand, it’s a little weird that it is treated differently from other errors (e.g. the per-file errors that get reported in cat). On the other hand, it makes writing the utilities more convenient as it eliminates boilerplate. At the moment, I would say if a utility takes multiple inputs and can keep going if one fails, then it should print out errors itself for each of the inputs (and then maybe keep track of the number of errors like in cat). If it takes one input or will fail if any of the inputs fails, then it should just return an error and let the framework machinery take care of printing.