Hi Arron,
On Tue, Apr 19, 2011 at 09:54:12PM -0400, [email protected] wrote:
> From: arron <[email protected]>
>
> __connman_dnsproxy_add_listener() can be used to add a listener to DNS proxy,
> so, besides the listener bound to lo device, listener bound to tether device
> can also
> be added. And tethering client can use tether as DNS server.
That looks pretty good to me. I have some minor comments:
> ---
> src/connman.h | 2 +
> src/dnsproxy.c | 186 ++++++++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 144 insertions(+), 44 deletions(-)
>
> diff --git a/src/connman.h b/src/connman.h
> index 824d31d..4e6b714 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -672,6 +672,8 @@ int __connman_iptables_commit(const char *table_name);
>
> int __connman_dnsproxy_init(void);
> void __connman_dnsproxy_cleanup(void);
> +int __connman_dnsproxy_add_listener(const char *interface);
> +void __connman_dnsproxy_remove_listener(const char *interface);
> int __connman_dnsproxy_append(const char *interface, const char *domain,
> const char *server);
> int __connman_dnsproxy_remove(const char *interface, const char *domain,
> const char *server);
> void __connman_dnsproxy_flush(void);
> diff --git a/src/dnsproxy.c b/src/dnsproxy.c
> index 0b2ff22..955ecbe 100644
> --- a/src/dnsproxy.c
> +++ b/src/dnsproxy.c
> @@ -112,17 +112,22 @@ struct request_data {
> gpointer name;
> gpointer resp;
> gsize resplen;
> + gchar *ifname;
> +};
> +
> +struct listener_data {
> + char *ifname;
> + GIOChannel *udp_listener_channel;
> + guint udp_listener_watch;
> + GIOChannel *tcp_listener_channel;
> + guint tcp_listener_watch;
> };
>
> static GSList *server_list = NULL;
> static GSList *request_list = NULL;
> static GSList *request_pending_list = NULL;
> static guint16 request_id = 0x0000;
> -
> -static GIOChannel *udp_listener_channel = NULL;
> -static guint udp_listener_watch = 0;
> -static GIOChannel *tcp_listener_channel = NULL;
> -static guint tcp_listener_watch = 0;
> +static GHashTable *listener_table = NULL;
>
> static int protocol_offset(int protocol)
> {
> @@ -215,19 +220,24 @@ static void send_response(int sk, unsigned char *buf,
> int len,
> static gboolean request_timeout(gpointer user_data)
> {
> struct request_data *req = user_data;
> + struct listener_data *ifdata;
>
> DBG("id 0x%04x", req->srcid);
>
> if (req == NULL)
> return FALSE;
>
> + ifdata = g_hash_table_lookup(listener_table, req->ifname);
> + if (ifdata == NULL)
> + return -ENODEV;
> +
> request_list = g_slist_remove(request_list, req);
> req->numserv--;
>
> if (req->resplen > 0 && req->resp != NULL) {
> int sk, err;
>
> - sk = g_io_channel_unix_get_fd(udp_listener_channel);
> + sk = g_io_channel_unix_get_fd(ifdata->udp_listener_channel);
>
> err = sendto(sk, req->resp, req->resplen, 0,
> &req->sa, req->sa_len);
> @@ -245,7 +255,8 @@ static gboolean request_timeout(gpointer user_data)
>
> hdr = (void *) (req->request);
> hdr->id = req->srcid;
> - sk = g_io_channel_unix_get_fd(udp_listener_channel);
> + sk = g_io_channel_unix_get_fd(
> + ifdata->udp_listener_channel);
> send_response(sk, req->request, req->request_len,
> &req->sa, req->sa_len, IPPROTO_UDP);
> }
> @@ -386,6 +397,7 @@ static int forward_dns_reply(unsigned char *reply, int
> reply_len, int protocol)
> struct domain_hdr *hdr;
> struct request_data *req;
> int dns_id, sk, err, offset = protocol_offset(protocol);
> + struct listener_data *ifdata;
>
> if (offset < 0)
> return offset;
> @@ -399,6 +411,10 @@ static int forward_dns_reply(unsigned char *reply, int
> reply_len, int protocol)
> if (req == NULL)
> return -EINVAL;
>
> + ifdata = g_hash_table_lookup(listener_table, req->ifname);
> + if (ifdata == NULL)
> + return -ENODEV;
> +
> DBG("id 0x%04x rcode %d", hdr->id, hdr->rcode);
>
> reply[offset] = req->srcid & 0xff;
> @@ -427,7 +443,7 @@ static int forward_dns_reply(unsigned char *reply, int
> reply_len, int protocol)
> request_list = g_slist_remove(request_list, req);
>
> if (protocol == IPPROTO_UDP) {
> - sk = g_io_channel_unix_get_fd(udp_listener_channel);
> + sk = g_io_channel_unix_get_fd(ifdata->udp_listener_channel);
> err = sendto(sk, req->resp, req->resplen, 0,
> &req->sa, req->sa_len);
> } else {
> @@ -1095,13 +1111,19 @@ static gboolean tcp_listener_event(GIOChannel
> *channel, GIOCondition condition,
> struct sockaddr_in6 client_addr;
> socklen_t client_addr_len = sizeof(client_addr);
> GSList *list;
> + struct listener_data *ifdata;
> + const char *interface = user_data;
>
> DBG("condition 0x%x", condition);
>
> + ifdata = g_hash_table_lookup(listener_table, interface);
> + if (ifdata == NULL)
> + return -ENODEV;
> +
> if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> - if (tcp_listener_watch > 0)
> - g_source_remove(tcp_listener_watch);
> - tcp_listener_watch = 0;
> + if (ifdata->tcp_listener_watch > 0)
> + g_source_remove(ifdata->tcp_listener_watch);
> + ifdata->tcp_listener_watch = 0;
>
> connman_error("Error with TCP listener channel");
>
> @@ -1113,7 +1135,7 @@ static gboolean tcp_listener_event(GIOChannel *channel,
> GIOCondition condition,
> client_sk = accept(sk, (void *)&client_addr, &client_addr_len);
> if (client_sk < 0) {
> connman_error("Accept failure on TCP listener");
> - tcp_listener_watch = 0;
> + ifdata->tcp_listener_watch = 0;
> return FALSE;
> }
>
> @@ -1151,6 +1173,7 @@ static gboolean tcp_listener_event(GIOChannel *channel,
> GIOCondition condition,
> buf[3] = req->dstid >> 8;
>
> req->numserv = 0;
> + req->ifname = (gchar *) interface;
> request_list = g_slist_append(request_list, req);
>
> for (list = server_list; list; list = list->next) {
> @@ -1216,10 +1239,16 @@ static gboolean udp_listener_event(GIOChannel
> *channel, GIOCondition condition,
> struct sockaddr_in6 client_addr;
> socklen_t client_addr_len = sizeof(client_addr);
> int sk, err, len;
> + struct listener_data *ifdata;
> + const char *interface = user_data;
> +
> + ifdata = g_hash_table_lookup(listener_table, interface);
> + if (ifdata == NULL)
> + return -ENODEV;
>
> if (condition & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> connman_error("Error with UDP listener channel");
> - udp_listener_watch = 0;
> + ifdata->udp_listener_watch = 0;
> return FALSE;
> }
>
> @@ -1262,16 +1291,17 @@ static gboolean udp_listener_event(GIOChannel
> *channel, GIOCondition condition,
> buf[1] = req->dstid >> 8;
>
> req->numserv = 0;
> + req->ifname = (gchar *) interface;
> req->timeout = g_timeout_add_seconds(5, request_timeout, req);
> request_list = g_slist_append(request_list, req);
>
> return resolv(req, buf, query);
> }
>
> -static int create_dns_listener(int protocol)
> +static int create_dns_listener(int protocol, const char *ifname)
> {
> GIOChannel *channel;
> - const char *ifname = "lo", *proto;
> + const char *proto;
> union {
> struct sockaddr sa;
> struct sockaddr_in6 sin6;
> @@ -1280,8 +1310,13 @@ static int create_dns_listener(int protocol)
> socklen_t slen;
> int sk, type, v6only = 0;
> int family = AF_INET6;
> + struct listener_data *ifdata;
>
> - DBG("");
> + DBG("interface %s", ifname);
> +
> + ifdata = g_hash_table_lookup(listener_table, ifname);
> + if (ifdata == NULL)
> + return -ENODEV;
>
> switch (protocol) {
> case IPPROTO_UDP:
> @@ -1361,62 +1396,79 @@ static int create_dns_listener(int protocol)
> g_io_channel_set_close_on_unref(channel, TRUE);
>
> if (protocol == IPPROTO_TCP) {
> - tcp_listener_channel = channel;
> - tcp_listener_watch = g_io_add_watch(channel,
> - G_IO_IN, tcp_listener_event, NULL);
> + ifdata->tcp_listener_channel = channel;
> + ifdata->tcp_listener_watch = g_io_add_watch(channel,
> + G_IO_IN, tcp_listener_event, (gpointer) ifname);
Here you're passing an ifname pointer that you have not allocated. It could
vanish on your back, so you should at least pass ifdata->ifname.
But as a matter of fact, we could do better here, and simply pass the ifdata
pointer as the user data. That would avoid the (cheap) hash table lookup in
the DNS resolving paths.
Also, by replacing the request_data->ifname pointer with a
request_data->ifdata pointer, we could also skip some of the hash table lookup
in the response sending path.
The rest of the code looks just fine to me.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
