From 8954b146ed904c63d8cf2bb91c0c2b65c2b7ec4e Mon Sep 17 00:00:00 2001 From: Santtu Lakkala Date: Tue, 24 Sep 2024 13:05:04 +0300 Subject: [PATCH] Fix clippy warnings, other cleanups --- client/src/client.rs | 42 ++++++++++++++----------------- client/src/endpoint.rs | 10 ++++---- common/src/query.rs | 26 +++++++++---------- common/src/types.rs | 44 ++++++++++++++++----------------- flake.nix | 7 +++--- internal/cmd/givc-agent/main.go | 2 +- src/admin/entry.rs | 18 +++++++------- src/admin/registry.rs | 18 +++++++++++--- src/admin/server.rs | 14 +++++------ src/bin/givc-cli.rs | 44 +++++++++++++++++++-------------- src/systemd_api/client.rs | 2 +- src/systemd_api/server.rs | 4 +-- src/utils/auth.rs | 2 +- src/utils/naming.rs | 2 +- 14 files changed, 123 insertions(+), 112 deletions(-) diff --git a/client/src/client.rs b/client/src/client.rs index 7a44d86..54450c2 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -4,26 +4,21 @@ use async_channel::Receiver; use givc_common::pb; pub use givc_common::query::{Event, QueryResult}; use givc_common::types::*; +use std::future::Future; +use std::pin::Pin; use tokio_stream::StreamExt; use tonic::transport::Channel; use tracing::debug; type Client = pb::admin_service_client::AdminServiceClient; -#[derive(Debug)] pub struct WatchResult { pub initial: Vec, // Design defence: we use `async-channel` here, as it could be used with both // tokio's and glib's eventloop, and recommended by gtk4-rs developers: pub channel: Receiver, - task: tokio::task::JoinHandle<()>, -} - -impl Drop for WatchResult { - fn drop(&mut self) { - self.task.abort() - } + pub task: Pin>>, } #[derive(Debug)] @@ -39,20 +34,20 @@ impl AdminClient { // New style api, not yet implemented, stub atm to make current code happy // FIXME: Still doubt if constructor should be sync or async - pub fn new(addr: String, port: u16, tls_info: Option<(String, TlsConfig)>) -> Self { - let (name, tls) = match tls_info { + pub fn new(address: String, port: u16, tls_info: Option<(String, TlsConfig)>) -> Self { + let (tls_name, tls) = match tls_info { Some((name, tls)) => (name, Some(tls)), None => (String::from("bogus(no tls)"), None), }; Self { endpoint: EndpointConfig { transport: TransportConfig { - address: addr, - port: port, + address, + port, protocol: String::from("bogus"), - tls_name: name, + tls_name, }, - tls: tls, + tls, }, } } @@ -92,8 +87,7 @@ impl AdminClient { vm_name, args, }; - let response = self.connect_to().await?.start_application(request).await?; - // Ok(response.into_inner().cmd_status) + let _response = self.connect_to().await?.start_application(request).await?; Ok(()) } pub async fn stop(&self, _app: String) -> anyhow::Result<()> { @@ -164,6 +158,8 @@ impl AdminClient { } pub async fn watch(&self) -> anyhow::Result { + use pb::admin::watch_item::Status; + use pb::admin::WatchItem; let (tx, rx) = async_channel::bounded::(10); let mut watch = self @@ -174,15 +170,13 @@ impl AdminClient { .into_inner(); let list = match watch.try_next().await? { - Some(first) => match first.status { - Some(pb::admin::watch_item::Status::Initial(init)) => QueryResult::parse_list(init.list)?, - Some(item) => bail!("Protocol error, first item in stream not pb::admin::watch_item::Status::Initial, {:?}", item), - None => bail!("Protocol error, initial item missing"), - }, + Some(WatchItem { status: Some(Status::Initial(init)) }) => QueryResult::parse_list(init.list)?, + Some(WatchItem { status: Some(item) }) => bail!("Protocol error, first item in stream not pb::admin::watch_item::Status::Initial, {:?}", item), + Some(_) => bail!("Protocol error, initial item missing"), None => bail!("Protocol error, status field missing"), }; - let task = tokio::task::spawn(async move { + let task = async move { loop { if let Ok(Some(event)) = watch.try_next().await { let event = match Event::try_from(event) { @@ -201,12 +195,12 @@ impl AdminClient { break; } } - }); + }; let result = WatchResult { initial: list, channel: rx, - task, + task: Box::pin(task), }; Ok(result) } diff --git a/client/src/endpoint.rs b/client/src/endpoint.rs index e22c9c3..7000b3a 100644 --- a/client/src/endpoint.rs +++ b/client/src/endpoint.rs @@ -25,11 +25,11 @@ pub struct EndpointConfig { impl TlsConfig { pub fn client_config(&self) -> anyhow::Result { - let pem = std::fs::read_to_string(self.ca_cert_file_path.as_os_str())?; + let pem = std::fs::read(&self.ca_cert_file_path)?; let ca = Certificate::from_pem(pem); - let client_cert = std::fs::read(self.cert_file_path.as_os_str())?; - let client_key = std::fs::read(self.key_file_path.as_os_str())?; + let client_cert = std::fs::read(&self.cert_file_path)?; + let client_key = std::fs::read(&self.key_file_path)?; let client_identity = Identity::from_pem(client_cert, client_key); let tls_name = self .tls_name @@ -42,8 +42,8 @@ impl TlsConfig { } pub fn server_config(&self) -> anyhow::Result { - let cert = std::fs::read(self.cert_file_path.as_os_str())?; - let key = std::fs::read(self.key_file_path.as_os_str())?; + let cert = std::fs::read(&self.cert_file_path)?; + let key = std::fs::read(&self.key_file_path)?; let identity = Identity::from_pem(cert, key); let config = ServerTlsConfig::new().identity(identity); Ok(config) diff --git a/common/src/query.rs b/common/src/query.rs index 875d026..e96897f 100644 --- a/common/src/query.rs +++ b/common/src/query.rs @@ -55,13 +55,13 @@ impl TryFrom for QueryResult { } } -impl Into for QueryResult { - fn into(self) -> pb::QueryListItem { - pb::QueryListItem { - name: self.name, - description: self.description, - vm_status: self.status.to_string(), - trust_level: self.trust_level.to_string(), +impl From for pb::QueryListItem { + fn from(val: QueryResult) -> Self { + Self { + name: val.name, + description: val.description, + vm_status: val.status.to_string(), + trust_level: val.trust_level.to_string(), } } } @@ -117,12 +117,12 @@ impl TryFrom for Event { } } -impl Into for Event { - fn into(self) -> pb::WatchItem { - match self { - Event::UnitRegistered(value) => Self::watch_item(Status::Added(value.into())), - Event::UnitStatusChanged(value) => Self::watch_item(Status::Updated(value.into())), - Event::UnitShutdown(value) => Self::watch_item(Status::Removed(value.into())), +impl From for pb::WatchItem { + fn from(val: Event) -> Self { + match val { + Event::UnitRegistered(value) => Event::watch_item(Status::Added(value.into())), + Event::UnitStatusChanged(value) => Event::watch_item(Status::Updated(value.into())), + Event::UnitShutdown(value) => Event::watch_item(Status::Removed(value.into())), } } } diff --git a/common/src/types.rs b/common/src/types.rs index 84ccdae..dfdb8cb 100644 --- a/common/src/types.rs +++ b/common/src/types.rs @@ -1,7 +1,7 @@ // This module contain literal translations of types from internal/pkgs/types/types.go // Some of them would be rewritten, replaced, or even removed use crate::pb; -use anyhow::{anyhow, bail}; +use anyhow::bail; use std::convert::{Into, TryFrom}; #[derive(Debug, Copy, Clone, PartialEq)] @@ -104,30 +104,30 @@ impl TryFrom for UnitType { // so we can use just Into trait // FIXME: Combination of `UnitType{ vm: Host, service: VM}` is ILLEGAL!!! // Should we use TryInto, or fix type system? -impl Into for UnitType { - fn into(self) -> u32 { +impl From for u32 { + fn from(val: UnitType) -> Self { use ServiceType::*; use VmType::*; - match self.vm { - Host => match self.service { + match val.vm { + Host => match val.service { Mgr => 0, Svc => 1, App => 2, VM => 100500, }, - AdmVM => match self.service { + AdmVM => match val.service { VM => 3, Mgr => 4, Svc => 5, App => 6, }, - SysVM => match self.service { + SysVM => match val.service { VM => 7, Mgr => 8, Svc => 9, App => 10, }, - AppVM => match self.service { + AppVM => match val.service { VM => 11, Mgr => 12, Svc => 13, @@ -172,14 +172,14 @@ impl TryFrom for UnitStatus { } } -impl Into for UnitStatus { - fn into(self) -> pb::UnitStatus { - pb::UnitStatus { - name: self.name, - description: self.description, - load_state: self.load_state, - active_state: self.active_state, - path: self.path, +impl From for pb::UnitStatus { + fn from(val: UnitStatus) -> Self { + Self { + name: val.name, + description: val.description, + load_state: val.load_state, + active_state: val.active_state, + path: val.path, } } } @@ -206,13 +206,13 @@ impl TryFrom for EndpointEntry { } } -impl Into for EndpointEntry { - fn into(self) -> pb::TransportConfig { +impl From for pb::TransportConfig { + fn from(val: EndpointEntry) -> Self { pb::TransportConfig { - protocol: self.protocol, - address: self.address, - port: self.port.to_string(), - name: self.tls_name, + protocol: val.protocol, + address: val.address, + port: val.port.to_string(), + name: val.tls_name, } } } diff --git a/flake.nix b/flake.nix index f6fc4a1..ba1af79 100644 --- a/flake.nix +++ b/flake.nix @@ -66,13 +66,14 @@ ./internal ]; }; - in - rec { - givc-agent = pkgs.callPackage ./nixos/packages/givc-agent.nix { inherit src; }; givc-admin-rs = pkgs.callPackage ./nixos/packages/givc-admin-rs.nix { inherit crane; src = ./.; }; + in + { + inherit givc-admin-rs; + givc-agent = pkgs.callPackage ./nixos/packages/givc-agent.nix { inherit src; }; givc-cli = givc-admin-rs.cli; }; }; diff --git a/internal/cmd/givc-agent/main.go b/internal/cmd/givc-agent/main.go index beffde0..51464a9 100644 --- a/internal/cmd/givc-agent/main.go +++ b/internal/cmd/givc-agent/main.go @@ -27,7 +27,7 @@ import ( func main() { var err error - log.Infof("Executing %s", filepath.Base(os.Args[0])) + log.Infof("Running %s", filepath.Base(os.Args[0])) name := os.Getenv("NAME") if name == "" { diff --git a/src/admin/entry.rs b/src/admin/entry.rs index e8dcb70..9ca8f04 100644 --- a/src/admin/entry.rs +++ b/src/admin/entry.rs @@ -2,7 +2,7 @@ // Some of them would be rewritten, replaced, or even removed use crate::pb; use anyhow::anyhow; -use std::convert::{Into, TryFrom}; +use std::convert::TryFrom; use givc_common::query::*; use givc_common::types::*; @@ -83,25 +83,25 @@ impl TryFrom for RegistryEntry { // Protocol very inconsistent here Ok(Self { name: req.name, - status: status, - watch: watch, + status, + watch, r#type: ty, placement: Placement::Endpoint(endpoint), }) } } -impl Into for RegistryEntry { - fn into(self) -> QueryResult { - let status = if self.status.is_running() { +impl From for QueryResult { + fn from(val: RegistryEntry) -> Self { + let status = if val.status.is_running() { VMStatus::Running } else { VMStatus::PoweredOff }; QueryResult { - name: self.name, - description: self.status.description, - status: status, + name: val.name, + description: val.status.description, + status, trust_level: TrustLevel::default(), } } diff --git a/src/admin/registry.rs b/src/admin/registry.rs index a1806d3..6cacab9 100644 --- a/src/admin/registry.rs +++ b/src/admin/registry.rs @@ -18,6 +18,12 @@ pub struct Registry { pubsub: broadcast::Sender, } +impl Default for Registry { + fn default() -> Self { + Self::new() + } +} + impl Registry { pub fn new() -> Self { Self { @@ -34,6 +40,7 @@ impl Registry { info!("Replaced old entry {:#?}", old); self.send_event(Event::UnitShutdown(old.into())) }; + info!("Sending event {event:?}"); self.send_event(event) } @@ -45,7 +52,10 @@ impl Registry { self.send_event(Event::UnitShutdown(entry.into())); Ok(()) } - None => bail!("Can't deregister entry {}, it not registered", name), + None => Err(anyhow!( + "Can't deregister entry {}, it not registered", + name + )), } } @@ -64,7 +74,7 @@ impl Registry { .filter(|x| x.starts_with(name)) .cloned() .collect(); - if list.len() == 0 { + if list.is_empty() { bail!("No entries match string {}", name) } else { Ok(list) @@ -115,7 +125,7 @@ impl Registry { e.status = status; self.send_event(Event::UnitStatusChanged(e.clone().into())) }) - .ok_or_else(|| anyhow!("Can't update state for {}, is not registered", name)) + .ok_or_else(|| anyhow!("Can't update state for {name}, is not registered")) } // FIXME: Should we dump full contents here for `query`/`query_list` high-level API @@ -156,7 +166,7 @@ mod tests { r.register(bar); assert!(r.contains(&foo_key)); - assert!(r.contains(&"bar".to_string())); + assert!(r.contains("bar")); let foo1 = r.by_name(&foo_key)?; assert_eq!(foo1, foo); diff --git a/src/admin/server.rs b/src/admin/server.rs index 9ea4286..b78ff5f 100644 --- a/src/admin/server.rs +++ b/src/admin/server.rs @@ -91,21 +91,21 @@ impl AdminServiceImpl { .agent() .with_context(|| "Resolving host agent".to_string())?; Ok(EndpointConfig { - transport: endpoint.into(), + transport: endpoint, tls: self.tls_config.clone(), }) } - pub fn agent_endpoint(&self, name: &String) -> anyhow::Result { - let endpoint = self.registry.by_name(&name)?.agent()?; + pub fn agent_endpoint(&self, name: &str) -> anyhow::Result { + let endpoint = self.registry.by_name(name)?.agent()?; Ok(EndpointConfig { - transport: endpoint.into(), + transport: endpoint, tls: self.tls_config.clone(), }) } pub fn app_entries(&self, name: String) -> anyhow::Result> { - if name.contains("@") { + if name.contains('@') { let list = self.registry.find_names(&name)?; Ok(list) } else { @@ -126,7 +126,7 @@ impl AdminServiceImpl { }; let tls_name = transport.tls_name.clone(); let endpoint = EndpointConfig { - transport: transport.into(), + transport, tls: self.tls_config.clone().map(|mut tls| { tls.tls_name = Some(tls_name); tls @@ -287,7 +287,7 @@ impl AdminServiceImpl { let app_entry = RegistryEntry { name: app_name, - status: status, + status, watch: true, r#type: UnitType { vm: VmType::AppVM, diff --git a/src/bin/givc-cli.rs b/src/bin/givc-cli.rs index aca6e65..35191ee 100644 --- a/src/bin/givc-cli.rs +++ b/src/bin/givc-cli.rs @@ -1,7 +1,8 @@ +use anyhow::anyhow; use clap::{Parser, Subcommand}; use givc::endpoint::TlsConfig; use givc::types::*; -use givc_client::AdminClient; +use givc_client::{client::WatchResult, AdminClient}; use serde::ser::Serialize; use std::path::PathBuf; use std::time; @@ -161,11 +162,11 @@ async fn main() -> std::result::Result<(), Box> { None => None, }; let reply = admin.query(ty, by_name).await?; - dump(&reply, as_json)? + dump(reply, as_json)? } Commands::QueryList { as_json } => { let reply = admin.query_list().await?; - dump(&reply, as_json)? + dump(reply, as_json)? } Commands::SetLocale { locale } => { @@ -175,28 +176,33 @@ async fn main() -> std::result::Result<(), Box> { Commands::SetTimezone { timezone } => { admin.set_timezone(timezone).await?; } + Commands::Watch { as_json, limit, - initial, + initial: dump_initial, } => { - let watch = admin.watch().await?; - let mut limit = limit; - - if initial { - dump(watch.initial.clone(), as_json)? + let WatchResult { + initial, + channel, + mut task, + } = admin.watch().await?; + let mut limit = limit.map(|l| 0..l); + + if dump_initial { + dump(initial, as_json)? } - loop { - let event = watch.channel.recv().await?; - dump(event, as_json)?; - if limit.as_mut().is_some_and(|l| { - *l -= 1; - *l == 0 - }) { - break; - } - } + tokio::select! { + res = async move { + // Change to Option::is_none_or() with rust >1.82 + while !limit.as_mut().is_some_and(|l| l.next().is_none()) { + dump(channel.recv().await?, as_json)?; + } + Ok(()) + } => res, + _ = task.as_mut() => Err(anyhow!("Watch task stopped unexpectedly")) + }? } }; diff --git a/src/systemd_api/client.rs b/src/systemd_api/client.rs index a260dff..e67271a 100644 --- a/src/systemd_api/client.rs +++ b/src/systemd_api/client.rs @@ -81,7 +81,7 @@ impl SystemDClient { ) -> anyhow::Result { let request = pb::systemd::AppUnitRequest { unit_name: unit, - args: args, + args, }; let resp = self.connect().await?.start_application(request).await?; let status = resp.into_inner(); diff --git a/src/systemd_api/server.rs b/src/systemd_api/server.rs index e9877a3..27542ed 100644 --- a/src/systemd_api/server.rs +++ b/src/systemd_api/server.rs @@ -4,12 +4,12 @@ use tonic::Status; pub use pb::systemd::unit_control_service_server::UnitControlServiceServer; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Default)] pub struct SystemdService {} impl SystemdService { pub fn new() -> Self { - Self {} + Default::default() } } diff --git a/src/utils/auth.rs b/src/utils/auth.rs index 25284a5..b2615c7 100644 --- a/src/utils/auth.rs +++ b/src/utils/auth.rs @@ -48,7 +48,7 @@ pub fn ensure_host(req: Request, hostname: &str) -> Result<(), Status> { } } -pub fn ensure_hosts(req: Request, hostnames: &Vec<&str>) -> Result<(), Status> { +pub fn ensure_hosts(req: Request, hostnames: &[&str]) -> Result<(), Status> { let permit = req .extensions() .get::() diff --git a/src/utils/naming.rs b/src/utils/naming.rs index d368278..6bad690 100644 --- a/src/utils/naming.rs +++ b/src/utils/naming.rs @@ -25,7 +25,7 @@ pub fn parse_service_name(name: &str) -> anyhow::Result<&str> { // From `agent` code, ported for future pub fn parse_application_name(name: &str) -> anyhow::Result<(&str, i32)> { if let Some(name_no_suffix) = name.strip_suffix(".service") { - if let Some((left, right)) = name_no_suffix.rsplit_once("@") { + if let Some((left, right)) = name_no_suffix.rsplit_once('@') { let num = right .parse::() .with_context(|| format!("While parsing number part of {}", name))?;