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.

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

gluten-mirage: fix read and writev functions #32

Merged
merged 3 commits into from
Aug 5, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .ocamlformat
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
profile = sparse
profile = default
break-cases = nested
break-fun-decl = smart
cases-exp-indent = 2
Expand Down
67 changes: 49 additions & 18 deletions mirage/gluten_mirage.ml
Original file line number Diff line number Diff line change
Expand Up @@ -32,54 +32,85 @@

open Lwt.Infix

module Make_IO (Flow : Mirage_flow.S) :
Gluten_lwt.IO with type socket = Flow.flow and type addr = unit = struct
type socket = Flow.flow

type addr = unit
type 'a socket = {
flow : 'a;
mutable buf : Cstruct.t;
}

let shutdown flow = Flow.close flow
let create_socket flow = { flow; buf = Cstruct.empty }

let shutdown_receive flow = Lwt.async (fun () -> shutdown flow)
module Make_IO (Flow : Mirage_flow.S) :
Gluten_lwt.IO with type socket = Flow.flow socket and type addr = unit =
struct
type nonrec socket = Flow.flow socket
type addr = unit

let shutdown sock = Flow.close sock.flow
let shutdown_receive sock = Lwt.async (fun () -> shutdown sock)
let close = shutdown

let read flow bigstring ~off ~len:_ =
let buffered_read sock len =
let trunc buf =
match Cstruct.length buf > len with
| false ->
buf
| true ->
let head, rest = Cstruct.split buf len in
sock.buf <- rest;
head
in
let buffered_data =
match Cstruct.is_empty sock.buf with
| true ->
None
| false ->
let buf = sock.buf in
sock.buf <- Cstruct.empty;
Some (Ok (`Data (trunc buf)))
in
match buffered_data with
| Some data ->
Lwt.return data
| None -> (
Flow.read sock.flow >|= fun data ->
assert (Cstruct.is_empty sock.buf);
match data with Ok (`Data buf) -> Ok (`Data (trunc buf)) | x -> x)

let read sock bigstring ~off ~len =
Lwt.catch
(fun () ->
Flow.read flow >|= function
buffered_read sock len >|= function
| Ok (`Data buf) ->
Bigstringaf.blit
buf.buffer
~src_off:buf.off
bigstring
~dst_off:off
Bigstringaf.blit buf.buffer ~src_off:buf.off bigstring ~dst_off:off
~len:buf.len;
`Ok buf.len
| Ok `Eof ->
`Eof
| Error error ->
failwith (Format.asprintf "%a" Flow.pp_error error))
(fun exn -> shutdown flow >>= fun () -> Lwt.fail exn)
(fun exn -> shutdown sock >>= fun () -> Lwt.fail exn)

let writev flow iovecs =
let writev sock iovecs =
let cstruct_iovecs =
List.map
(fun { Faraday.buffer; off; len } ->
Cstruct.of_bigarray ~off ~len buffer)
Comment on lines 99 to 100
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
(fun { Faraday.buffer; off; len } ->
Cstruct.of_bigarray ~off ~len buffer)
(fun { Faraday.buffer; off; len } ->
let copy = Bigstringaf.copy ~off ~len buffer in
Cstruct.of_bigarray ~off:0 ~len copy)

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 that (potentially) allocates many small Cstructs, my version allocates only one bigger Cstruct and copies everything into it.

iovecs
in
let len = Cstruct.lenv cstruct_iovecs in
Copy link
Owner

Choose a reason for hiding this comment

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

It should be easier to create the copy inside the List.map function above. I'll propose a suggestion.

let data = Cstruct.create_unsafe len in
let _, _ = Cstruct.fillv ~src:cstruct_iovecs ~dst:data in
Comment on lines +103 to +105
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let len = Cstruct.lenv cstruct_iovecs in
let data = Cstruct.create_unsafe len in
let _, _ = Cstruct.fillv ~src:cstruct_iovecs ~dst:data in

Lwt.catch
(fun () ->
Flow.writev flow cstruct_iovecs >|= fun x ->
Flow.write sock.flow data >|= fun x ->
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Flow.write sock.flow data >|= fun x ->
Flow.write sock.flow cstruct_iovecs >|= fun x ->

match x with
| Ok () ->
`Ok (Cstruct.lenv cstruct_iovecs)
| Error `Closed ->
`Closed
| Error other_error ->
raise (Failure (Format.asprintf "%a" Flow.pp_write_error other_error)))
(fun exn -> shutdown flow >>= fun () -> Lwt.fail exn)
(fun exn -> shutdown sock >>= fun () -> Lwt.fail exn)
end

module Server (Flow : Mirage_flow.S) = Gluten_lwt.Server (Make_IO (Flow))
Expand Down
8 changes: 6 additions & 2 deletions mirage/gluten_mirage.mli
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@
* POSSIBILITY OF SUCH DAMAGE.
*---------------------------------------------------------------------------*)

type 'a socket

val create_socket : 'a -> 'a socket

module Server (Flow : Mirage_flow.S) :
Gluten_lwt.Server with type socket = Flow.flow and type addr = unit
Gluten_lwt.Server with type socket = Flow.flow socket and type addr = unit

module Client (Flow : Mirage_flow.S) :
Gluten_lwt.Client with type socket = Flow.flow
Gluten_lwt.Client with type socket = Flow.flow socket