Return of ipmi

[mchf]

Problem

There used to be support for Ipmi reporting (#2490) which was lost during transformation of manager service from ruby to rust.

Solution

Ipmi code rewritten from ruby to rust and hooked into the manager service

TODO

• changelog (will be added once we get close to end of the review ;-))

Testing

• Tested manually

[imobachgs]

I have a few comments about the overall approach.

You are using an empty struct which does not hold any kind of state. Actually, the &self arguments are not needed (e.g., check is_available).

I guess you want to somehow group all those functions together and in the future you might want to keep some state (e.g., caching is_available or something like that).

Additionally, you are using some hard-coded values (two paths). What about moving those paths to the struct itself?

struct Ipmi {
  tool: PathBuf, // feel free to use better names
  dev: PathBuf
}

impl Ipmi {
  fn new(tool: &str, dev: &str) -> Self { // you can use a String if you prefer
    Self {
      tool: tool.to_string(),
      dev: dev.to_string()
    }
  }
}

impl Default for Impi {
  fn default() -> Self {
    Self::new("/usr/bin/ipmitool", "/dev/ipmi0")
  }
}

This approach would enable you to write unit tests (that are actually missing).

[imobachgs]

What about returning an error if the ipmi is not available? In that case, this struct does not have any usage. I do not have a strong opinion about this, it is just a suggestion.

[mchf]

Well ipmi is kind of optional from our POV and in fact we do not care whether it is available or not ... And wrapping return value into a Result would make coding little bit more noisy ;-)

[imobachgs]

Yes, I understand, that's why I say it is just a suggestion. All those functions are now a no-op. At least I would log in the send_command function that IPMI is not available. Or if you prefer, you could call is_available when the manager starts printing whether it is supported or not.

[mchf]

I have a few comments about the overall approach.

You are using an empty struct which does not hold any kind of state. Actually, the &self arguments are not needed (e.g., check is_available).

I guess you want to somehow group all those functions together and in the future you might want to keep some state (e.g., caching is_available or something like that).

yes, that was the idea. However originally I wanted to do a (kind of) 1:1 version of original ruby code. As it wasn't according to your taste I've added basic update according to your comments

[imobachgs]

I am still not convinced by the no-op and, more important, the lack of tests.

But please, do not merge until you can try a full installation.

Please, add changes entry.

[imobachgs]

Suggested change

	from ruby to rust.
	from Ruby to Rust.