From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E07CC61CAC; Wed, 26 Jul 2023 15:41:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C222E7A9F; Wed, 26 Jul 2023 15:41:24 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS; Wed, 26 Jul 2023 15:41:23 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 94A2245974; Wed, 26 Jul 2023 15:41:22 +0200 (CEST) Date: Wed, 26 Jul 2023 15:41:21 +0200 From: Wolfgang Bumiller To: Lukas Wagner Cc: pve-devel@lists.proxmox.com, pbs-devel@lists.proxmox.com Message-ID: References: <20230726125006.616124-1-l.wagner@proxmox.com> <20230726125006.616124-3-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230726125006.616124-3-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.115 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [error.rs, access.rs, rest.rs, h2service.rs, lib.rs] Subject: Re: [pve-devel] [pbs-devel] [PATCH proxmox 2/5] router: rest-server: auth-api: use new http-error crate X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Jul 2023 13:41:24 -0000 I'd like `proxmox-router` to keep re-exporting both the `http_bail/err` macros and the `HttpError` type. This would require much fewer changes at all the call sites, and we don't need to explicitly depend on the new crate everywhere. Its point is mostly that perlmod code doesn't need to pull the router into our perl code base. On Wed, Jul 26, 2023 at 02:50:03PM +0200, Lukas Wagner wrote: > Signed-off-by: Lukas Wagner > --- > proxmox-auth-api/Cargo.toml | 2 ++ > proxmox-auth-api/src/api/access.rs | 3 +- > proxmox-rest-server/Cargo.toml | 1 + > proxmox-rest-server/src/h2service.rs | 2 +- > proxmox-rest-server/src/rest.rs | 2 +- > proxmox-router/Cargo.toml | 1 + > proxmox-router/src/error.rs | 44 ---------------------------- > proxmox-router/src/lib.rs | 7 +---- > 8 files changed, 9 insertions(+), 53 deletions(-) > delete mode 100644 proxmox-router/src/error.rs > > diff --git a/proxmox-auth-api/Cargo.toml b/proxmox-auth-api/Cargo.toml > index bd1884a..45d7cbc 100644 > --- a/proxmox-auth-api/Cargo.toml > +++ b/proxmox-auth-api/Cargo.toml Effectively this crate wouldn't need to be touched at all. > @@ -28,6 +28,7 @@ serde = { workspace = true, optional = true, features = [ "derive" ] } > serde_json = { workspace = true, optional = true } > serde_plain = { workspace = true, optional = true } > > +proxmox-http-error = { workspace = true, optional = true } > proxmox-rest-server = { workspace = true, optional = true } > proxmox-router = { workspace = true, optional = true } > proxmox-schema = { workspace = true, optional = true, features = [ "api-macro", "api-types" ] } > @@ -45,6 +46,7 @@ api = [ > "dep:http", > "dep:serde_json", > > + "dep:proxmox-http-error", > "dep:proxmox-rest-server", > "dep:proxmox-router", > "dep:proxmox-tfa", > diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs > index 428d22a..d9e11f4 100644 > --- a/proxmox-auth-api/src/api/access.rs > +++ b/proxmox-auth-api/src/api/access.rs > @@ -4,7 +4,8 @@ use anyhow::{bail, format_err, Error}; > use serde_json::{json, Value}; > > use proxmox_rest_server::RestEnvironment; > -use proxmox_router::{http_err, Permission, RpcEnvironment}; > +use proxmox_router::{Permission, RpcEnvironment}; > +use proxmox_http_error::http_err; > use proxmox_schema::{api, api_types::PASSWORD_SCHEMA}; > use proxmox_tfa::api::TfaChallenge; > > diff --git a/proxmox-rest-server/Cargo.toml b/proxmox-rest-server/Cargo.toml > index 8daf1d3..4bf2bca 100644 > --- a/proxmox-rest-server/Cargo.toml > +++ b/proxmox-rest-server/Cargo.toml Nor this one. > @@ -38,6 +38,7 @@ url.workspace = true > proxmox-async.workspace = true > proxmox-compression.workspace = true > proxmox-http = { workspace = true, optional = true } > +proxmox-http-error.workspace = true > proxmox-io.workspace = true > proxmox-lang.workspace = true > proxmox-router.workspace = true > diff --git a/proxmox-rest-server/src/h2service.rs b/proxmox-rest-server/src/h2service.rs > index 84a3098..08636bc 100644 > --- a/proxmox-rest-server/src/h2service.rs > +++ b/proxmox-rest-server/src/h2service.rs > @@ -8,7 +8,7 @@ use std::task::{Context, Poll}; > use futures::*; > use hyper::{Body, Request, Response, StatusCode}; > > -use proxmox_router::http_err; > +use proxmox_http_error::http_err; > use proxmox_router::{ApiResponseFuture, HttpError, Router, RpcEnvironment}; > > use crate::formatter::*; > diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs > index cffcedc..8db570e 100644 > --- a/proxmox-rest-server/src/rest.rs > +++ b/proxmox-rest-server/src/rest.rs > @@ -22,11 +22,11 @@ use tokio::time::Instant; > use tower_service::Service; > use url::form_urlencoded; > > +use proxmox_http_error::{http_bail, http_err}; > use proxmox_router::{ > check_api_permission, ApiHandler, ApiMethod, HttpError, Permission, RpcEnvironment, > RpcEnvironmentType, UserInformation, > }; > -use proxmox_router::{http_bail, http_err}; > use proxmox_schema::{ObjectSchemaType, ParameterSchema}; > > use proxmox_async::stream::AsyncReaderStream; basically all of the above could be dropped, and only the remaining parts below together with re-exports are required. Unless I'm missing something. > diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml > index 34a1985..7d4d32f 100644 > --- a/proxmox-router/Cargo.toml > +++ b/proxmox-router/Cargo.toml > @@ -26,6 +26,7 @@ rustyline = { version = "9", optional = true } > libc = { workspace = true, optional = true } > > proxmox-lang.workspace = true > +proxmox-http-error.workspace = true > proxmox-schema.workspace = true > proxmox-async.workspace = true > > diff --git a/proxmox-router/src/error.rs b/proxmox-router/src/error.rs > deleted file mode 100644 > index e285cf7..0000000 > --- a/proxmox-router/src/error.rs > +++ /dev/null > @@ -1,44 +0,0 @@ > -use std::fmt; > - > -#[doc(hidden)] > -pub use http::StatusCode; > - > -/// HTTP error including `StatusCode` and message. > -#[derive(Debug)] > -pub struct HttpError { > - pub code: StatusCode, > - pub message: String, > -} > - > -impl std::error::Error for HttpError {} > - > -impl HttpError { > - pub fn new(code: StatusCode, message: String) -> Self { > - HttpError { code, message } > - } > -} > - > -impl fmt::Display for HttpError { > - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { > - write!(f, "{}", self.message) > - } > -} > - > -/// Macro to create a HttpError inside a anyhow::Error > -#[macro_export] > -macro_rules! http_err { > - ($status:ident, $($fmt:tt)+) => {{ > - ::anyhow::Error::from($crate::HttpError::new( > - $crate::error::StatusCode::$status, > - format!($($fmt)+) > - )) > - }}; > -} > - > -/// Bail with an error generated with the `http_err!` macro. > -#[macro_export] > -macro_rules! http_bail { > - ($status:ident, $($fmt:tt)+) => {{ > - return Err($crate::http_err!($status, $($fmt)+)); > - }}; > -} > diff --git a/proxmox-router/src/lib.rs b/proxmox-router/src/lib.rs > index 7f4f2d9..1bb5026 100644 > --- a/proxmox-router/src/lib.rs > +++ b/proxmox-router/src/lib.rs > @@ -5,11 +5,6 @@ pub mod format; > #[cfg(feature = "cli")] > pub mod cli; > > -// this is public so the `http_err!` macro can access `http::StatusCode` through it > -#[doc(hidden)] > -#[cfg(feature = "server")] > -pub mod error; > - > mod permission; > mod router; > mod rpc_environment; > @@ -17,7 +12,7 @@ mod serializable_return; > > #[doc(inline)] > #[cfg(feature = "server")] > -pub use error::HttpError; > +pub use proxmox_http_error::HttpError; > > pub use permission::*; > pub use router::*; > -- > 2.39.2