c-cube / tiny_httpd

Minimal HTTP server using good old threads + blocking IO, with a small request router.
https://c-cube.github.io/tiny_httpd
75 stars 11 forks source link

Make client_addr of Request.t public #74

Closed barti2du closed 8 months ago

barti2du commented 8 months ago

Would it be possible to expose the client_addr of Request.t?

AFAIU, it could boil down to

--- a/src/Tiny_httpd_server.ml  Thu Dec 19 14:06:40 2023 +0200
+++ b/src/Tiny_httpd_server.ml  Thu Dec 19 14:07:56 2023 +0200
@@ -176,6 +176,7 @@

   let headers self = self.headers
   let host self = self.host
+  let client_addr self = self.client_addr
   let meth self = self.meth
   let path self = self.path
   let body self = self.body
diff -r 24640471ca81 src/Tiny_httpd_server.mli
--- a/src/Tiny_httpd_server.mli Thu Dec 19 14:06:40 2023 +0200
+++ b/src/Tiny_httpd_server.mli Thu Dec 19 14:07:56 2023 +0200
@@ -127,6 +127,9 @@
   val host : _ t -> string
   (** Host field of the request. It also appears in the headers. *)

+  val client_addr : _ t -> Unix.sockaddr
+  (** Client address field of the request. *)
+
   val meth : _ t -> Meth.t
   (** Method for the request. *)

The primary use case would be access logging.

However, I may be missing something obvious... Is there another way to extract the remote address of the client?

c-cube commented 8 months ago

no, that's a possibility. You can open a PR with this patch :). I'd also like a @since NEXT_RELEASE in the mli file's comment.

vphantom commented 8 months ago

Wait, I'm already logging the client IP address in my Tiny_httpd test program. It's in Tiny_httpd.Request.t's client_addr. I guess I added it and forgot to upload the patch? 🤔 Hm Git says no.

Do you still have branch client-ip? That's where that change was according to my old history. Look around August 8th. My commit was 53182375c0cdb on August 7th and afterwards we removed the option part on August 8th in 03596c1a08f9b9.

vphantom commented 8 months ago

It was pull request #71 . It says "merged manually" because I think I messed up some merging on my end. Maybe that's why it somehow didn't make it to the current head?

vphantom commented 8 months ago

I just took a second look and the client IP address is indeed exposed at https://github.com/c-cube/tiny_httpd/blob/d40a0070cbc62eb9995f46d11be12462d956e1b8/src/Tiny_httpd_server.ml#L167

barti2du commented 8 months ago

I just took a second look and the client IP address is indeed exposed at

https://github.com/c-cube/tiny_httpd/blob/d40a0070cbc62eb9995f46d11be12462d956e1b8/src/Tiny_httpd_server.ml#L167

@vphantom It is there, but it is hidden by the mli (Request.t is private).

vphantom commented 8 months ago

That is actually not what private means in OCaml: it means that external modules cannot create instances of it, but it is freely readable by all. Very handy. That's how I'm able to log the client IP and such internals easily in my test server.

barti2du commented 8 months ago

@vphantom shame on me, indeed I can access it like req.Tiny_httpd.Request.client_addr where req is Request.t.

I would still prefer to have functional access to client_addr for consistency with other fields (like meth, host, path, etc) , but it is largely subjective.

barti2du commented 8 months ago

@vphantom , @c-cube: Thank You very much :)