Skip to content

Conversation

@jonludlam
Copy link
Collaborator

Signed-off-by: Jon Ludlam jonathan.ludlam@citrix.com

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

I believe this is a faithful translation to the monadic syntax plus some re-structuring. Luckily compilation is a good test that nothing important is missed and so I believe that this is quite a safe change.

proxy fd (Lwt_unix.of_unix_file_descr proxy_socket) in
let t_unix =
lwt fd, peer = Lwt_unix.accept s_unix in
Lwt_unix.accept s_unix >>= fun (fd, peer) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the use of a pair for (fd, peer) is faithful to the original but not necessary. The function could simply take to parameters: fun fd peer ->.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that Lwt_unix.accept returns a pair, so sadly we cannot change that if I'm not mistaken:

val accept : file_descr -> (file_descr * sockaddr) Lwt.t

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct.

let buffer = String.make (String.length token) '\000' in
let io_vector = Lwt_unix.io_vector ~buffer ~offset:0 ~length:(String.length buffer) in
lwt n, fds = Lwt_unix.recv_msg ~socket:fd ~io_vectors:[io_vector] in
Lwt_unix.recv_msg ~socket:fd ~io_vectors:[io_vector] >>= fun (n, fds) ->
Copy link
Contributor

@lindig lindig Feb 8, 2017

Choose a reason for hiding this comment

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

Same as above==: fun n fds ->

*)

open Sexplib.Std
include Xenops_types.TopLevel
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of include because it introduces a lot of names unqualified into the current scope. I understand why it is here, though.

@lindig lindig merged commit 8bf3ea9 into xapi-project:master Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants