unix: add Cstruct_unix.{read,write,writev,send,recv} by djs55 · Pull Request #302 · mirage/ocaml-cstruct

djs55 · 2022-10-26T14:24:21Z

This is useful for direct-style code to avoid copying Cstructs into temporary byte buffers to use the Unix. API.

Signed-off-by: David Scott dave@recoil.org

This is useful for direct-style code to avoid copying Cstructs into temporary byte buffers. Signed-off-by: David Scott <dave@recoil.org>

haesbaert · 2022-10-26T14:35:16Z

Me gusta ! I've been carrying this around for ages: https://github.com/haesbaert/rawlink/blob/520207190814ebfc0db9fd4f6d9c6e0f1297732d/lib/rawlink_stubs.c#L466

unix/recv_stubs.c

avsm · 2022-10-26T14:47:23Z

unix/unix_cstruct.ml

+    | remaining ->
+      (* write at most iov_max at a time *)
+      let to_send = first iov_max [] remaining in
+      let n = stub_writev fd (List.map (fun x -> x.Cstruct.buffer, x.Cstruct.off, x.Cstruct.len) to_send) in


do you expand to a tuple here because the Cstruct.t record type might change? Given this is in the same repository, it's probably safe enough to just pass the Cstruct record in directly to the FFI and save some allocations

Yes that was it -- I was using these stubs in another repo. I'll drop the tuple since we're here.

haesbaert · 2022-10-26T15:09:32Z

unix/recv_stubs.c

+    int fd = Int_val(val_fd);
+
+    caml_release_runtime_system();
+    n = recv(fd, buf, len, 0);


I'm not sure a recv with null flags is very useful, it becomes just read(2), (don't know about windows though).

Good point, I’ve copied the flags code from your stubs :)

haesbaert · 2022-10-26T15:11:57Z

unix/recv_stubs.c

+    val_ofs = Field(val_c, 1);
+    val_len = Field(val_c, 2);
+
+    void *buf = (void *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);


minor bikeshedding, would move buf to the beginning of the block, declarations after statement is not valid C89 (not sure if anyone cares these days)
Also void * arithmetic is technically undefined behaviour (also not sure if anyone cares).

I think void * is used here to be in-sync with what recv expects:

ssize_t recv(int sockfd, void *buf, size_t len, int flags);

Ack but passing pointer of any type to something that expects a void pointer is ok, my only bit was that we're doing (void *)++, which is undefined behavior (but every compiler implicitly assumes char *) so it's ok.

https://stackoverflow.com/questions/3922958/void-arithmetic says it's a GCC extension, and attempting this should produce warnings.

haesbaert · 2022-10-26T15:13:08Z

unix/send_stubs.c

+
+    SOCKET s = Socket_val(val_fd);
+    caml_release_runtime_system();
+    n = send(s, buf, len, 0);


same as recv

haesbaert · 2022-10-26T15:13:37Z

unix/write_stubs.c

+  val_len = Field(val_c, 2);
+  void *buf = (char *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);
+  size_t len = Long_val(val_len);
+  int n = 0;


should be ssize_t

haesbaert · 2022-10-26T15:15:19Z

unix/writev_stubs.c

+#include <limits.h>
+#endif
+
+CAMLprim


bikeshedding, the other stubs don't break line here

Still breaking line here in stub_cstruct_iov_max

unix/read_stubs.c

unix/unix_cstruct.ml

haesbaert · 2022-10-27T10:46:02Z

I can provide Cstruct_unix.recvfrom and Cstruct_unix.sendto if it's desirable, I just can't do the windows bits.

unix/writev_stubs.c

@haesbaert

Linux doesn't export IOV_MAX unless you define _XOPEN_SOURCE or _GNU_SOURCE (from @haesbaert) Co-authored-by: Christiano Haesbaert <haesbaert@haesbaert.org>

@avsm

makes it easier to grep for the source of the error if we just name it after the function. (from @avsm) Co-authored-by: Anil Madhavapeddy <anil@recoil.org>

The recv/send tests looked a bit complex so I've changed mine, don't see why we would need threads since it's just one message.

avsm · 2022-11-01T14:53:17Z

unix/unix_cstruct.mli

    If only a partial receive is possible, the return argument is now many bytes were received. *)
+
+val recvfrom: Unix.file_descr -> Cstruct.t -> Unix.msg_flag list -> int * Unix.sockaddr
+(** [recvfrom fd c] Like Unix.recvfrom, but for Cstruct. *)


Suggested change

(** [recvfrom fd c] Like Unix.recvfrom, but for Cstruct. *)

(** [recvfrom fd c] Like {! Unix.recvfrom}, but for Cstruct. *)

will link to it on the ocaml.org docs then.

avsm · 2022-11-01T14:54:10Z

unix/recvfrom_stubs.c

+  caml_acquire_runtime_system();
+
+  if (n == -1)
+    caml_uerror("recvfrom", Nothing);


Suggested change

caml_uerror("recvfrom", Nothing);

caml_uerror("cstruct_recvfrom", Nothing);

I do find these Unix_errors are marginally more useful if you can grep for the function they came from

Signed-off-by: David Scott <dave@recoil.org>

- `caml_uerror` is defined in ocaml/ocaml#67a4d75cfafa040099cdd322e23464362359af29 and is in 5.0+ - for backwards compat ocaml 5.0 has #define uerror caml_uerror Signed-off-by: David Scott <dave@recoil.org>

This allows compat with OCaml 4.13 Signed-off-by: David Scott <dave@recoil.org>

Windows socket handles and error codes are managed differently. Signed-off-by: David Scott <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

There's no need to repack it as a tuple for the C bindings because the definition is in this repository. Signed-off-by: David Scott <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

Back to vi... Signed-off-by: David Scott <dave@recoil.org>

This became redundant when the re-packing of a Cstruct.t as a tuple was removed. Signed-off-by: David Scott <dave@recoil.org>

Signed-off-by: David Scott <dave@recoil.org>

avsm · 2022-11-03T09:51:23Z

unix/recv_stubs.c

+#endif
+
+static int msg_flag_table[] = {
+    MSG_OOB, MSG_DONTROUTE, MSG_PEEK /* XXX */


what's the XXX for here? Incomplete flags?

Cause we assume how stdlib ordered things and also because we redefine them, would be nice if stdlib exposed them as not static so we could just use caml_msg_flag_table or something

The stdlib does expose them -- as OCaml values. Just add a type definition in unix_cstruct.mli instead and get rid of the XXX:

type msg_flag = Unix.msg_flag = MSG_OOB | MSG_DONTROUTE | MSG_PEEK

This will create an alias to Unix.msg_flag that also checks that it's exactly the same as what is specified there.
Try for example, swapping two of the flags:

type t = Unix.msg_flag = MSG_OOB | MSG_PEEK | MSG_DONTROUTE;; Error: This variant or record definition does not match that of type Unix.msg_flag Constructors MSG_DONTROUTE and MSG_PEEK have been swapped.

Ah that's smart ! yes we should do that.

avsm · 2022-11-03T09:54:57Z

unix/unix_cstruct.ml

+let iov_max = stub_iov_max ()
+
+(* return the first n fragments, suitable for writev *)
+let rec first n rev_acc = function


I mildly dislike having to do these potentially O(n) list operations for a writev, and mandate that in the external interface. In your vpnkit usecase, would you be able to pass in a Cstruct.t array instead for writev, and then just call writev with simple O(1) time slices instead?

It affects the interface we expose in the mli, hence my asking now (as opposed to optimising later)

haesbaert · 2022-11-03T13:28:10Z

unix/write_stubs.c

+  val_buf = Field(val_c, 0);
+  val_ofs = Field(val_c, 1);
+  val_len = Field(val_c, 2);
+  void *buf = (char *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);


Suggested change

void *buf = (char *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);

uint8_t *buf = (uint8_t *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);

I approve this change, the FFI about bigarray says that it's an uint8_t array. We should keep this assumption on the C side too.

haesbaert · 2022-11-03T13:30:32Z

unix/writev_stubs.c

+#include <limits.h>
+#endif
+
+CAMLprim


Still breaking line here in stub_cstruct_iov_max

haesbaert · 2022-11-03T13:32:26Z

unix/writev_stubs.c

+    length++;
+  /* Only copy up to the iovec array size */
+  if (length > IOV_MAX)
+    length = IOV_MAX;


Silently truncating it might be worse than just letting the syscall failing later on in uerror with EINVAL.
You exposed the maximum, if the user goes above he should be punished with an exception imho.
It's a bit harmless for a STREAM that has to handle shortcounts, but a DATAGRAM will send half a packet.
(even if we're all a bit insane because IOV_MAX is huge)

We should mostly mirror what writev(2) itself does, and that returns an EINVAL if > IOV_MAX. So I agree that a uerror should be raised if it's too large, and not a silent truncation.

haesbaert · 2022-11-03T13:35:15Z

unix/writev_stubs.c

+    val_buf = Field(head, 0);
+    val_ofs = Field(head, 1);
+    val_len = Field(head, 2);
+    iovec[i].iov_base = (char *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);


Suggested change

iovec[i].iov_base = (char *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);

iovec[i].iov_base = (uint8_t *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);

dinosaure · 2023-02-02T12:08:39Z

A release should be done soon but this PR has some remaining issues. As far as I can say, it's mostly about style. @djs55 can you do a review of them?

patricoferris · 2023-02-02T13:23:27Z

Just thought I would mention that I will try to get a pread/pwrite/preadv/pwritev PR open soonish. I started hacking on that over here 4fd8e39 but don't currently have a windows machine to do that part and couldn't get macOS (11+) to be happy about the preadv/pwritev functions. I mention this because it would help with the potential IOCP/kqueue backends in Eio. Just thought I would mention it in case others were already hacking on this :))

patricoferris · 2023-02-10T16:46:15Z

I haven't worked out a nice story for supporting pread/pwrite on Windows because the calls to ReadFile/WriteFile with an Overlapped changes the current position (pread/pwrite do not change the position). Lwt_unix has decided to leave that as undefined behaviour (see ocsigen/lwt#768). Would that be okay here?

dinosaure · 2023-02-14T11:26:38Z

Would that be okay here?

I think it's ok when the module explicitely mention unix in it's name 🙂 . A note (like lwt) will be cool to inform the user if he/she tries to use this module on Windows.

unix: add Cstruct_unix.{read,write,writev,send,recv}

99f6a02

This is useful for direct-style code to avoid copying Cstructs into temporary byte buffers. Signed-off-by: David Scott <dave@recoil.org>

djs55 mentioned this pull request Oct 26, 2022

Add support for AF_UNIX SOCK_DGRAM moby/vpnkit#602

Open

avsm reviewed Oct 26, 2022

View reviewed changes

unix/recv_stubs.c Outdated Show resolved Hide resolved

avsm reviewed Oct 26, 2022

View reviewed changes

haesbaert reviewed Oct 26, 2022

View reviewed changes

unix/send_stubs.c Outdated

SOCKET s = Socket_val(val_fd);

caml_release_runtime_system();

n = send(s, buf, len, 0);

Copy link

Member

haesbaert Oct 26, 2022

Choose a reason for hiding this comment

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

same as recv

haesbaert reviewed Oct 26, 2022

View reviewed changes

unix/read_stubs.c Show resolved Hide resolved

dinosaure reviewed Oct 27, 2022

View reviewed changes

unix/unix_cstruct.ml Outdated Show resolved Hide resolved

haesbaert reviewed Oct 28, 2022

View reviewed changes

unix/writev_stubs.c Show resolved Hide resolved

haesbaert mentioned this pull request Oct 28, 2022

Add recvfrom/sendto to #302 (writev_haesbaert) #303

Open

djs55 and others added 3 commits November 1, 2022 14:22

Update unix/writev_stubs.c

87382a1

Linux doesn't export IOV_MAX unless you define _XOPEN_SOURCE or _GNU_SOURCE (from @haesbaert) Co-authored-by: Christiano Haesbaert <haesbaert@haesbaert.org>

Update unix/recv_stubs.c

6002721

makes it easier to grep for the source of the error if we just name it after the function. (from @avsm) Co-authored-by: Anil Madhavapeddy <anil@recoil.org>

recvfrom and sendto with flags

2390134

The recv/send tests looked a bit complex so I've changed mine, don't see why we would need threads since it's just one message.

avsm reviewed Nov 1, 2022

View reviewed changes

djs55 added 10 commits November 1, 2022 15:55

windows: ifdef for socket headers

f70b3ac

Signed-off-by: David Scott <dave@recoil.org>

use uerror rather than unix_error(errno,...) or caml_uerror

7d77f0a

- `caml_uerror` is defined in ocaml/ocaml#67a4d75cfafa040099cdd322e23464362359af29 and is in 5.0+ - for backwards compat ocaml 5.0 has #define uerror caml_uerror Signed-off-by: David Scott <dave@recoil.org>

Use old backwards-compat alloc_sockaddr, get_sockaddr

04e45e2

This allows compat with OCaml 4.13 Signed-off-by: David Scott <dave@recoil.org>

add Windows support for Unix sendto/recvfrom

3e6d7e4

Windows socket handles and error codes are managed differently. Signed-off-by: David Scott <dave@recoil.org>

Unix_cstruct.send should take a flag list

284960a

Signed-off-by: David Scott <dave@recoil.org>

Unix_cstruct.recv should take a flag list.

ec807a7

Signed-off-by: David Scott <dave@recoil.org>

we can assume knowledge of Cstruct.t

fa5ffe7

There's no need to repack it as a tuple for the C bindings because the definition is in this repository. Signed-off-by: David Scott <dave@recoil.org>

Move declarations before statements, use uint8_t* for buf

2cf81f3

Signed-off-by: David Scott <dave@recoil.org>

send/recv/read/write return ssize_t

4a04478

Signed-off-by: David Scott <dave@recoil.org>

Remove strange linebreak inserted by VSCode

44ec8fc

Back to vi... Signed-off-by: David Scott <dave@recoil.org>

djs55 added 4 commits November 2, 2022 10:14

Remove unnecessary type buffer

803ba0c

This became redundant when the re-packing of a Cstruct.t as a tuple was removed. Signed-off-by: David Scott <dave@recoil.org>

Improve .mli by linking to the Unix functions

aeca686

Signed-off-by: David Scott <dave@recoil.org>

In Unix_error, include the C stub function name stub_cstruct_foo

5920235

Signed-off-by: David Scott <dave@recoil.org>

$ dune build --auto-promote @fmt

471ca03

Signed-off-by: David Scott <dave@recoil.org>

djs55 requested review from avsm, dinosaure, haesbaert and talex5 November 3, 2022 07:55

avsm reviewed Nov 3, 2022

View reviewed changes

haesbaert reviewed Nov 3, 2022

View reviewed changes

patricoferris mentioned this pull request Apr 27, 2023

Posix-based windows implementation ocaml-multicore/eio#497

Merged

patricoferris mentioned this pull request May 8, 2023

Windows support ocaml-multicore/eio#125

Open

9 tasks

	(** [recvfrom fd c] Like Unix.recvfrom, but for Cstruct. *)
	(** [recvfrom fd c] Like {! Unix.recvfrom}, but for Cstruct. *)

	caml_uerror("recvfrom", Nothing);
	caml_uerror("cstruct_recvfrom", Nothing);

	void buf = (char )Caml_ba_data_val(val_buf) + Long_val(val_ofs);
	uint8_t buf = (uint8_t )Caml_ba_data_val(val_buf) + Long_val(val_ofs);

	iovec[i].iov_base = (char *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);
	iovec[i].iov_base = (uint8_t *)Caml_ba_data_val(val_buf) + Long_val(val_ofs);

Comments

Conversation

djs55 commented Oct 26, 2022

Uh oh!

haesbaert commented Oct 26, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haesbaert Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

haesbaert commented Oct 27, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dinosaure commented Feb 2, 2023

Uh oh!

patricoferris commented Feb 2, 2023

Uh oh!

patricoferris commented Feb 10, 2023

Uh oh!

dinosaure commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

haesbaert Oct 26, 2022 •

edited

Loading