public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH backup] client: use build_authority in build_uri
@ 2021-05-06  6:55 Wolfgang Bumiller
  2021-05-06  8:30 ` [pbs-devel] applied: " Dietmar Maurer
  0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2021-05-06  6:55 UTC (permalink / raw)
  To: pbs-devel

so we don't need to also duplicate the IPv6 bracket logic

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
Just a little cleanup since we added 2 different bracket codes for ipv6
in the recent commits...

 src/client/http_client.rs | 31 +++++++++++++------------------
 src/tools/http.rs         |  2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/src/client/http_client.rs b/src/client/http_client.rs
index f2fada23..785626e8 100644
--- a/src/client/http_client.rs
+++ b/src/client/http_client.rs
@@ -26,7 +26,10 @@ use crate::tools::{
     self,
     BroadcastFuture,
     DEFAULT_ENCODE_SET,
-    http::HttpsConnector,
+    http::{
+        build_authority,
+        HttpsConnector,
+    },
 };
 
 /// Timeout used for several HTTP operations that are expected to finish quickly but may block in
@@ -274,23 +277,15 @@ fn load_ticket_info(prefix: &str, server: &str, userid: &Userid) -> Option<(Stri
 }
 
 fn build_uri(server: &str, port: u16, path: &str, query: Option<String>) -> Result<Uri, Error> {
-    let path = path.trim_matches('/');
-    let bytes = server.as_bytes();
-    let len = bytes.len();
-    let uri = if len > 3 && bytes.contains(&b':') && bytes[0] != b'[' && bytes[len-1] != b']' {
-        if let Some(query) = query {
-            format!("https://[{}]:{}/{}?{}", server, port, path, query)
-        } else {
-            format!("https://[{}]:{}/{}", server, port, path)
-        }
-    } else {
-        if let Some(query) = query {
-            format!("https://{}:{}/{}?{}", server, port, path, query)
-        } else {
-            format!("https://{}:{}/{}", server, port, path)
-        }
-    };
-    Ok(uri.parse()?)
+    let builder = Uri::builder()
+        .scheme("https")
+        .authority(build_authority(server, port)?);
+    match query {
+        Some(query) => builder.path_and_query(format!("{}?{}", path, query)),
+        None => builder.path_and_query(path),
+    }
+    .build()
+    .map_err(Error::from)
 }
 
 impl HttpClient {
diff --git a/src/tools/http.rs b/src/tools/http.rs
index cfdd9b16..9adfc118 100644
--- a/src/tools/http.rs
+++ b/src/tools/http.rs
@@ -29,7 +29,7 @@ use crate::tools::{
 };
 
 // Build a http::uri::Authority ("host:port"), use '[..]' around IPv6 addresses
-fn build_authority(host: &str, port: u16) -> Result<Authority, Error> {
+pub(crate) fn build_authority(host: &str, port: u16) -> Result<Authority, Error> {
     let bytes = host.as_bytes();
     let len = bytes.len();
     let authority = if len > 3 && bytes.contains(&b':') && bytes[0] != b'[' && bytes[len-1] != b']' {
-- 
2.20.1





^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup] client: use build_authority in build_uri
@ 2021-05-06  7:01 Dietmar Maurer
  2021-05-06  7:14 ` Wolfgang Bumiller
  0 siblings, 1 reply; 6+ messages in thread
From: Dietmar Maurer @ 2021-05-06  7:01 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Wolfgang Bumiller

In general a good idea, but we now merge code
from two independent http client implementation (making them
dependent)?

 
> On 05/06/2021 8:55 AM Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
>  
> so we don't need to also duplicate the IPv6 bracket logic
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> Just a little cleanup since we added 2 different bracket codes for ipv6
> in the recent commits...
> 
>  src/client/http_client.rs | 31 +++++++++++++------------------
>  src/tools/http.rs         |  2 +-
>  2 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/src/client/http_client.rs b/src/client/http_client.rs
> index f2fada23..785626e8 100644
> --- a/src/client/http_client.rs
> +++ b/src/client/http_client.rs
> @@ -26,7 +26,10 @@ use crate::tools::{
>      self,
>      BroadcastFuture,
>      DEFAULT_ENCODE_SET,
> -    http::HttpsConnector,
> +    http::{
> +        build_authority,
> +        HttpsConnector,
> +    },
>  };
>  
>  /// Timeout used for several HTTP operations that are expected to finish quickly but may block in
> @@ -274,23 +277,15 @@ fn load_ticket_info(prefix: &str, server: &str, userid: &Userid) -> Option<(Stri
>  }
>  
>  fn build_uri(server: &str, port: u16, path: &str, query: Option<String>) -> Result<Uri, Error> {
> -    let path = path.trim_matches('/');
> -    let bytes = server.as_bytes();
> -    let len = bytes.len();
> -    let uri = if len > 3 && bytes.contains(&b':') && bytes[0] != b'[' && bytes[len-1] != b']' {
> -        if let Some(query) = query {
> -            format!("https://[{}]:{}/{}?{}", server, port, path, query)
> -        } else {
> -            format!("https://[{}]:{}/{}", server, port, path)
> -        }
> -    } else {
> -        if let Some(query) = query {
> -            format!("https://{}:{}/{}?{}", server, port, path, query)
> -        } else {
> -            format!("https://{}:{}/{}", server, port, path)
> -        }
> -    };
> -    Ok(uri.parse()?)
> +    let builder = Uri::builder()
> +        .scheme("https")
> +        .authority(build_authority(server, port)?);
> +    match query {
> +        Some(query) => builder.path_and_query(format!("{}?{}", path, query)),
> +        None => builder.path_and_query(path),
> +    }
> +    .build()
> +    .map_err(Error::from)
>  }
>  
>  impl HttpClient {
> diff --git a/src/tools/http.rs b/src/tools/http.rs
> index cfdd9b16..9adfc118 100644
> --- a/src/tools/http.rs
> +++ b/src/tools/http.rs
> @@ -29,7 +29,7 @@ use crate::tools::{
>  };
>  
>  // Build a http::uri::Authority ("host:port"), use '[..]' around IPv6 addresses
> -fn build_authority(host: &str, port: u16) -> Result<Authority, Error> {
> +pub(crate) fn build_authority(host: &str, port: u16) -> Result<Authority, Error> {
>      let bytes = host.as_bytes();
>      let len = bytes.len();
>      let authority = if len > 3 && bytes.contains(&b':') && bytes[0] != b'[' && bytes[len-1] != b']' {
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel




^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup] client: use build_authority in build_uri
@ 2021-05-06  7:34 Dietmar Maurer
  2021-05-06  7:43 ` Wolfgang Bumiller
  0 siblings, 1 reply; 6+ messages in thread
From: Dietmar Maurer @ 2021-05-06  7:34 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: Proxmox Backup Server development discussion

> On 05/06/2021 9:14 AM Wolfgang Bumiller <w.bumiller@proxmox.com> wrote:
> 
>  
> On Thu, May 06, 2021 at 09:01:46AM +0200, Dietmar Maurer wrote:
> > In general a good idea, but we now merge code
> > from two independent http client implementation (making them
> > dependent)?
> 
> I mean... it's called "tools::http"... as in "http utilities", which
> IMO does not really sound like "this is an independent http client,
> which must not be reused"... ;-)

What naming do you suggest instead?

> And it's a rather thin layer around hyper (the *actual* client...) to
> add proxy support which, too, is something that we may at some point
> want to reuse without the rest of `SimpleHttp`, no?

I am not sure if it is worth to reuse that for that backup client. We simply
do not need proxy support there.




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-06  8:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06  6:55 [pbs-devel] [PATCH backup] client: use build_authority in build_uri Wolfgang Bumiller
2021-05-06  8:30 ` [pbs-devel] applied: " Dietmar Maurer
2021-05-06  7:01 [pbs-devel] " Dietmar Maurer
2021-05-06  7:14 ` Wolfgang Bumiller
2021-05-06  7:34 Dietmar Maurer
2021-05-06  7:43 ` Wolfgang Bumiller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal