Hi Andrew,
Thanks for the patch -- and thanks for including a man-page update and
a test update;
I've scattered a few minor comments below.
Also, it turns out that your patch actually exposes a small existing
problem -- the man
page doesn't match the code for ares_set_socket_callback() on error
returns -- so I'll
get that fixed.
Thanks,
David
On Sat, Feb 6, 2016 at 7:05 PM, Andrew Ayer <agwa_at_andrewayer.name> wrote:
>
> I just submitted the following Pull Request:
>
> https://github.com/c-ares/c-ares/pull/36
>
> I've included the patch below.
>
> Thanks,
> Andrew
>
>
> From 40c12d323c7b76449aab6c9fc490b579aef48e71 Mon Sep 17 00:00:00 2001
> From: Andrew Ayer <agwa_at_andrewayer.name>
> Date: Sat, 6 Feb 2016 09:20:41 -0800
> Subject: [PATCH] Add ares_set_socket_configure_callback()
>
> This function sets a callback that is invoked after the socket is
> created, but before the connection is established. This is an ideal
> time to customize various socket options.
>
[snip]
>
> diff --git a/ares_process.c b/ares_process.c
> index c3ac77b..0325f51 100644
> --- a/ares_process.c
> +++ b/ares_process.c
> @@ -1031,6 +1031,17 @@ static int open_tcp_socket(ares_channel channel, struct server_state *server)
> }
> #endif
>
> + if (channel->sock_config_cb)
> + {
> + int err = channel->sock_config_cb(s, SOCK_STREAM,
> + channel->sock_config_cb_data);
> + if (err < 0)
> + {
> + sclose(s);
> + return err;
> + }
> + }
> +
> /* Connect to the server. */
> if (connect(s, sa, salen) == -1)
> {
> @@ -1115,6 +1126,17 @@ static int open_udp_socket(ares_channel channel, struct server_state *server)
> return -1;
> }
>
> + if (channel->sock_config_cb)
> + {
> + int err = channel->sock_config_cb(s, SOCK_DGRAM,
> + channel->sock_config_cb_data);
> + if (err < 0)
> + {
> + sclose(s);
> + return err;
> + }
> + }
> +
> /* Connect to the server. */
> if (connect(s, sa, salen) == -1)
> {
> diff --git a/ares_set_socket_configure_callback.3 b/ares_set_socket_configure_callback.3
> new file mode 100644
> index 0000000..f89737e
> --- /dev/null
> +++ b/ares_set_socket_configure_callback.3
> @@ -0,0 +1,29 @@
> +.\"
> +.TH ARES_SET_SOCKET_CONFIGURE_CALLBACK 3 "6 Feb 2016"
> +.SH NAME
> +ares_set_socket_configure_callback \- Set a socket configuration callback
> +.SH SYNOPSIS
> +.nf
> +.B #include <ares.h>
> +.PP
> +.B void ares_set_socket_configure_callback(ares_channel \fIchannel\fP,
> + ares_sock_config_callback \fIcallback\fP,
> + void *\fIuserdata\fP)
> +.PP
> +.B cc file.c -lcares
> +.fi
> +.SH DESCRIPTION
> +.PP
> +This function sets a \fIcallback\fP in the given ares channel handle. This
> +callback function will be invoked after the socket has been created, but
> +before it has been connected to the remote server, which is an ideal time
> +to configure various socket options. The callback must return ARES_SUCCESS
> +if things are fine, or use the standard ares error codes to signal errors
> +back.
(Pre-existing problem) This doesn't match the code behavior -- ares error
codes are ignored, only values <0 signal errors.
I'll fix the existing man page, but this should be updated too.
>
> Returned errors will abort the ares operation.
> +.SH SEE ALSO
> +.BR ares_init_options (3)
Be nice to add a see-also link to ares_set_socket_callback, and from
there to here.
>
> +.SH AVAILABILITY
> +ares_set_socket_configure_callback(3) was added in c-ares 1.11.0
> +.SH AUTHOR
> +Andrew Ayer
> +
> diff --git a/test/ares-test-mock.cc b/test/ares-test-mock.cc
> index 15f0abc..aab9560 100644
> --- a/test/ares-test-mock.cc
> +++ b/test/ares-test-mock.cc
> @@ -137,6 +137,35 @@ TEST_P(MockChannelTest, SockCallback) {
> EXPECT_EQ("{'www.google.com' aliases=[] addrs=[2.3.4.5]}", ss.str());
> }
>
> +static int sock_create_cb_count = 0;
> +static int SocketCreateCallback(ares_socket_t fd, int type, void *data) {
> + if (verbose) std::cerr << "SocketCreateCallback(" << fd << ") invoked" << std::endl;
> + sock_create_cb_count++;
> + return ARES_SUCCESS;
> +}
I've updated the code this is copied from so there's a test for a
failing callback
(commit 87d641a907b7); be nice to add the same here.
>
> +
> +TEST_P(MockChannelTest, SockCreateCallback) {
> + DNSPacket rsp;
> + rsp.set_response().set_aa()
> + .add_question(new DNSQuestion("www.google.com", ns_t_a))
> + .add_answer(new DNSARR("www.google.com", 100, {2, 3, 4, 5}));
> + EXPECT_CALL(server_, OnRequest("www.google.com", ns_t_a))
> + .WillOnce(SetReply(&server_, &rsp));
> +
> + // Get notified of new sockets
> + ares_set_socket_callback(channel_, SocketCreateCallback, nullptr);
Isn't this testing the existing entrypoint, not the new one?
>
> +
> + HostResult result;
> + sock_create_cb_count = 0;
> + ares_gethostbyname(channel_, "www.google.com.", AF_INET, HostCallback, &result);
> + Process();
> + EXPECT_EQ(1, sock_create_cb_count);
> + EXPECT_TRUE(result.done_);
> + std::stringstream ss;
> + ss << result.host_;
> + EXPECT_EQ("{'www.google.com' aliases=[] addrs=[2.3.4.5]}", ss.str());
> +}
> +
> // TCP only to prevent retries
> TEST_P(MockTCPChannelTest, MalformedResponse) {
> std::vector<byte> one = {0x01};
Received on 2016-02-09