General idiom usage

[TheSignPainter98]

Hi there! I took a quick look through this codebase and have a few friendly pointers to help get the most out of using Rust :) I think at the moment, much of the code I've seen feels as though it's been translated from another language such as C and although they can occupy much the same space, idiomatic Rust and C are quite different.

Please note that these things I spotted come from a sample of the files, so some impressions may be spurious. I also grouped these into one issue to avoid spam, so apologies for the length. Anyway, in no particular order, here's what I found:

• There seem to be quite a few places there's _ = fallible_func() is used, unexpectedly preventing error propagation. Whilst this may be okay in tests, it's definitely a code smell in general
• The proxy_agent cli argument parser is hand-rolled. Unless case insensitivity is required, I think it would be easier to maintain and more idiomatic to derive a parser with clap
• Some fallible functions return booleans instead of a Result. This isn't idiomatic and has the added downside that the user can quietly ignore the error condition! By using a Result instead, the compiler will enforce that the error is considered, thus leading to more robust code. (e.g. here, I'd expect an error detailing what went wrong)
• I see very many free functions and although these are sometimes okay, most instances I've seen would be more idiomatically expressed as methods. E.g.

  fn attach_kprobe_program(bpf: &mut Bpf, ...)

  Would feel more idiomatic as

  impl Bpf {
      fn attach_kprobe_program(&mut self, ...)
  }

  It may be necessary to have a wrapper type for Bpf in this case as it is defined in the aya crate, but the newtype pattern, i.e. struct Bpf(aya::Bpf);, could help here

• Sometimes values where the type system (correctly) expresses invalid values get converted to valid-looking ones with invalid values. Specifically here, where if proxy_agent_exe is not valid utf8, path_to_string returns 'InvalidPath,' which the OS then attempts to execute.

  execute_command(&path_to_string(proxy_agent_exe), vec!["--version"], -1)

  Assuming this fails, later on, the string 'Unknown' is returned to indicate an error, which isn't very helpful. Checking whether the proxy_agent_exe is valid utf8 before executing would improve this greatly and give enough information for a more appropriate error message. Another approach could be to use camino::Utf8PathBuf rather than std::path::PathBuf to represent proxy_agent_exe, which would express the opinion that incoming paths must be valid utf8. This may be quite a strong opinion though, so I'm not sure (I lack context)

• The shared state seems to always be cloned before use as many functions involving it unnecessarily take ownership (e.g. this one). As ownership doesn't need to be moved, a reference would be more idiomatic here.
• Whenever SharedState is used, it seems to be behind an Arc<Mutex<_>>, which:
  • Should probably be hidden by a new type as this leaks implementation details of how it's locked
  • probably shouldn't lock the entire struct, as SharedState contains a very large number of fields so one thread will exclude all other threads from reading the shared state, which seems like it would cause unnecessary stalls. Instead, it may be better to lock individual fields or group fields together

• I see std::io::Result being used as a custom result type (a common rookie error seen for example here), but really crates should define their own error and result types, leading to a nice 'branded' feeling API (e.g. in crate foo, foo::bar() might return a foo::Result which can hold a foo::Error). This is surprisingly straightforward to remedy, the usual way this is done is by using thiserror deriving a custom error type and making a new Result alias as follows (I like to put these next to one another in lib.rs

  #[derive(Debug, thiserror::Error)]
  pub enum Error {
      #[error("cannot foo")]
      Foo,
  }
  
  pub type Result<T> = std::result::Result<T, Error>

  Doing this also has the added benefit of encouraging better error handling and

• There are a couple of places where the use of new enums would be more robust, e.g. ModuleState has associated string constants, which doesn't prevent other arbitrary states from being used where a module state is expected, leading to the rather awkward and very fragile need to use a comment!

  pub status: String,  // ModuleState, RUNNING|STOPPED

[ZhidongPeng]

Thanks for your feedback list, I will fix them gradually with priority.