Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CP-49647 use URI to build URIs #5650

Merged
merged 14 commits into from
Jun 4, 2024
Merged

CP-49647 use URI to build URIs #5650

merged 14 commits into from
Jun 4, 2024

Conversation

lindig
Copy link
Contributor

@lindig lindig commented May 24, 2024

For improved IPv6 compatibility, use URI to build URIs. In particular, this will take care of bracketing IPv6 addresses in URIs.

psafont
psafont reviewed May 24, 2024
View reviewed changes
ocaml/xapi/xapi_vm_migrate.ml
@@ -399,10 +399,14 @@ let pool_migrate ~__context ~vm ~host ~options =
use_compression ~__context options (Helpers.get_localhost ~__context) host
in
debug "%s using stream compression=%b" __FUNCTION__ compress ;
let ip = Http.Url.maybe_wrap_IPv6_literal address in
let scheme = if !Xapi_globs.migration_https_only then "https" else "http" in
let http = if !Xapi_globs.migration_https_only then "https" else "http" in
Copy link
Member

@psafont psafont May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be scheme, because it can be either 'http' or 'https'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But scheme inside URI is bound and shadows it when using it in URI.(make ... scheme). So I would have to give up opening URI to use scheme.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that make it simpler?

Uri.(
      make ~scheme ~host:...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with Uri.(..) is that scheme and so on are functions inside Uri that shadow my bindings. So now I prefer to not open Uri and avoid this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see... In the following commit you have Uri.make ... |> Uri.to_string to avoid this.

psafont
psafont reviewed May 24, 2024
View reviewed changes
ocaml/libs/http-lib/http.ml
| Http h, data ->
let userpassat =
(* this should have file:// *)
Printf.sprintf "file:%s%s" path (data_to_string data)
Copy link
Member

@psafont psafont May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Uri can be used here as well, and in all cases, hopefully.

I'd try to generate the parameters for Uri.make:

let scheme, host, port, userinfo, path, query = match url with
  | File {path}, data ->
       "file", None, None, None, (Printf.sprintf "%s%s" path (data_to_string data)), None
  ...
in
Uri.(make ~scheme ~host ~port ~userinfo ~path ~query |> to_string)

Copy link
Contributor Author

@lindig lindig May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code above does not work because scheme, host and so on become shadowed once Uri is open:

utop # let host = "localhost" in Uri.(make ~host () |> to_string);;
Error: This expression has type t -> string option
       but an expression was expected of type string

But could use Uri.make and Uri.to_string explicitly.

@lindig lindig marked this pull request as ready for review May 28, 2024 12:27
@lindig lindig force-pushed the private/christianlin/CP-49647 branch from 132d580 to 3439839 Compare May 28, 2024 13:06
Christian Lindig added 12 commits June 3, 2024 14:22
CP-49647 use URI for create_misc
75f483c 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for dbsync_master
b739ca3 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for export.ml
ddf75c7 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for import.ml
c4b796b 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for importexport.ml
63bc136 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for rrd_proxy.ml
a9c40a0 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for sm_fs_ops.ml
001a1e6 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for xapi_message.ml
65ffdd5 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for xapi_xenops.ml
4353d67 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for xapi_vm_migrate.ml
9c4857e 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for xapi_host.ml
2a24c48 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for cli_util.ml
960b5b1 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
@lindig lindig force-pushed the private/christianlin/CP-49647 branch from 3439839 to c1e84a2 Compare June 3, 2024 13:38
@lindig lindig requested a review from robhoes June 3, 2024 13:41
@lindig
Copy link
Contributor Author

lindig commented Jun 3, 2024

This has gone through BVT/BST
I have eliminated one commit that caused a failure - will address this separately
Remaining work will go into CP-49677

robhoes
robhoes reviewed Jun 3, 2024
View reviewed changes
ocaml/xe-cli/newcli.ml Outdated
let ( let* ) = Option.bind in
let* scheme = Uri.scheme uri in
let* host = Uri.host uri in
let path = Uri.path uri in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this now include the query string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops - this commit should not be included; will take a look

Christian Lindig added 2 commits June 3, 2024 14:49
CP-49647 use URI for http.ml
47834b5 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
CP-49647 use URI for cli_operations
2b59034 
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
@lindig lindig force-pushed the private/christianlin/CP-49647 branch 2 times, most recently from 76afca7 to 2b59034 Compare June 3, 2024 14:47
@lindig lindig requested a review from contificate June 4, 2024 10:10
robhoes
robhoes reviewed Jun 4, 2024
View reviewed changes
ocaml/xapi/xapi_host.ml
Printf.sprintf "%s://%s/" scheme
(Http.Url.maybe_wrap_IPv6_literal master_address)
in
let master_url = Uri.make ~scheme ~host:master_address () |> Uri.to_string in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to omit the trailing /. That is probably not an issue, but we should check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on testing it's not an issue.

contificate
contificate approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@contificate contificate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, assuming all the aforementioned implicit shadowing captures are mitigated.

@lindig
Copy link
Contributor Author

lindig commented Jun 4, 2024

Shadowing is caught by the type checker because it's simple values versus functions.

@lindig lindig merged commit 1c4f3a9 into xapi-project:master Jun 4, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psafont psafont psafont left review comments

@robhoes robhoes robhoes approved these changes

@contificate contificate contificate approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lindig @robhoes @psafont @contificate