* 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; 5+ 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] 5+ messages in thread* Re: [pbs-devel] [PATCH backup] client: use build_authority in build_uri
2021-05-06 7:01 [pbs-devel] [PATCH backup] client: use build_authority in build_uri Dietmar Maurer
@ 2021-05-06 7:14 ` Wolfgang Bumiller
0 siblings, 0 replies; 5+ messages in thread
From: Wolfgang Bumiller @ 2021-05-06 7:14 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: Proxmox Backup Server development discussion
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"... ;-)
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?
^ permalink raw reply [flat|nested] 5+ 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; 5+ 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] 5+ 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, 0 replies; 5+ messages in thread
From: Wolfgang Bumiller @ 2021-05-06 7:43 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: Proxmox Backup Server development discussion
On Thu, May 06, 2021 at 09:34:03AM +0200, Dietmar Maurer wrote:
> > 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?
I suppose I'd put shared http stuff in `tools::http` and if the client
should be separate I'd suggest `tools::http::client`, but I'm not
convinced it cannot stay as is while sharing some code in the first
place
>
> > 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.
yet ;-)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [pbs-devel] [PATCH backup] client: use build_authority in build_uri
@ 2021-05-06 6:55 Wolfgang Bumiller
0 siblings, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2021-05-06 7:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 7:01 [pbs-devel] [PATCH backup] client: use build_authority in build_uri Dietmar Maurer
2021-05-06 7:14 ` Wolfgang Bumiller
-- strict thread matches above, loose matches on Subject: below --
2021-05-06 7:34 Dietmar Maurer
2021-05-06 7:43 ` Wolfgang Bumiller
2021-05-06 6:55 Wolfgang Bumiller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.