[illumos-utils] Use tokio::process::Command, not std::process::Command

[smklein]

Many commands in illumos-utils bottomed out in a call to "std::process::Command", eventually
exec-ing a process. These commands were often used from asynchronous code, even though they
could block the calling thread.

This PR converts many - but not all - of these calls to use tokio::process::Command instead, which
no longer blocks the calling thread.

[smklein]

To confirm: is it critical that the snapshot blocks the drop method? Async drop does not exist in rust.

This API, as implemented, creates the background task but does not await for it to complete.

[smklein]

The changes in this file - and others within illumos-utils/src/... - are representative of "what actually is changing in this PR":

• std::process::Command -> tokio::process::Command
• This makes the function async
• The rest of the changes are fall-out from this, identifying blocks of code as asynchronous

[papertigers]

I don't think it's critical that the drop method is synchronous however it does introduce a race condition in the test here, we may be able to work around it.

[smklein]

I've changed the behavior here to use a more explicit drop, and log errors if this wasn't handled. I also tried to refactor above to make this case less likely -- cleaning up snapshots at a higher-level in the call to get_zone_logs

[jgallagher]

This is fine, but I'm curious (and maybe this is a "Friday afternoon Rust chat" question more than a PR question) - is there a reason to prefer async_trait over returning impl Future<Output = ...> + Send now that Rust supports that?

[smklein]

I think this is slightly more painful for default methods - I see elsewhere we have the pattern of:

trait Api {
  fn foobar(&self) -> impl Future<Output=Baz> + Send;
}

impl Api for MyStruct {
  async fn foobar(&self) -> Baz { ... }
}

But if I try to use a default impl of:

trait Api {
  fn foobar(&self) -> impl Future<Output=Baz> + Send {
     // Do anything async
  }
}

I'll get the error the ".await" is only allowed in async blocks, and foobar isn't an async block.

[jgallagher]

Might be able to get away with

trait Api {
    fn foobar(&self) -> impl Future<Output=Baz> + Send {
        async {
            // Do anything async
        }
    }
}

but that might also be kind of a pain if you're doing anything with self. Anyway - it's a good point; async_trait does seem to be smoother still, at least for this case.

[jgallagher]

Do we need this redundant closure? (If so maybe worth a short comment explaining why)

[smklein]

I'll avoid it by pulling this out to a new method instead. We're using it to make translation of the error type more convenient.

[jgallagher]

Should this be set after Zfs::destroy_snapshot() returns Ok(_)? (As written the caller couldn't retry destruction, although maybe that doesn't matter at the moment.)

[smklein]

Sure, I'll move it down. I don't think we're trying to call this again from any pub codepaths, but I don't see a problem with making it retryable either.