Hi Forest, On Wed, Jul 07, 2010 at 10:51:58PM -0400, Forest Bond wrote: > Hi Samuel, > > On Thu, Jul 08, 2010 at 01:44:59AM +0200, Samuel Ortiz wrote: > > On Tue, Jul 06, 2010 at 11:13:42PM -0400, Forest Bond wrote: > > > You'll notice that this patch does not take into consideration your > > > recommendation that nameserver host routes be added in > > > __connman_service_append_nameserver. The reason for this is that the > > > gateway > > > often not known when that function is called. > > In which case is the gateway not known at that point ? > > In the DHCP case, ipv4_probe is called by dhcp_bound, i.e. after we actually > > received a complete DHCP offer. If you don't have a gateway at that point > > you > > won't get it I suppose. > > In the manual case, if you don't have a gateway, that means the user haven't > > set it and there's not much you can do about it. > > > > Could you please describe how you're not getting a gateway there, I > > may be missing a point ? > > I can explain as well as I understand it. My conclusion is based on what I am > seeing in testing, which is not necessarily what is supposed to be happening > by > design. > > Here are the relevant call sequences as I understand them: > > The gateway is added to the routing table like this: > > connman_dhcp_bound > `-> ipv4_probe > `-> __connman_service_append_nameserver <- You are here. > `-> connection_probe > `-> __connman_service_indicate_state (READY) > `-> update_nameservers > `-> connman_resolver_append <- Nameservers go into > effect here. > `-> set_default_gateway > `-> connman_inet_set_gateway_address <- Gateway is added to > the routing table > here. > > The gateway is stored in service->ipconfig like this: > > rtnl_newroute > `-> process_newroute > `-> __connman_ipconfig_newroute <- Gateway is stored > in > service->ipconfig. > > So, to answer your question, yes the gateway is known when > __connman_service_append_nameserver is called. But it is not yet stored in > service->ipconfig, so in most cases this expression returns NULL: > > __connman_ipconfig_get_gateway(connman_network_get_index(service->network)) True, I was thinking that you would go through the element stuff to retrieve the gateway. But your remark below about us not catching the DNS manual setting path makes the above solution invalid.
> As I see it, either the connman's late storage of the gateway in > service->ipconfig is wrong, or my assumption that I should be getting the > gateway using __connman_ipconfig_get_gateway is wrong. I think the ipdevice->gateway setting is correct, I'd rather not change this part. > If connman is handling the gateway incorrectly, it could be changed in a few > different ways: > > * Perhaps the service should not have state READY until after the gateway has > been confirmed set via rtnl. > * Maybe the gateway should be stored in the ipconfig structure in a different > place (in connection_probe, for instance). > > Or, we could leave connman's gateway handling logic alone and I could just get > the gateway by searching the element tree for the connection element and > getting > it using connman_element_get_value like connection_probe does now. > > However, there is another problem with adding the route in > __connman_service_append_nameserver. User-specified nameservers > (service->nameservers) are not set that way, they are set in set_property > (service.c) directly. Shouldn't we also install host routes for these > nameservers? We certainly should, yes. So, I am attaching 2 patches to this email: 0001-Factorize-host-route-setting-routine.patch that cleans the inet.c host route setting stuff and that should go upstream anyway. And 0002-Set-DNS-host-routes-before-toggling-the-service-READ.patch that basically sets the DNS host routes before we set the service to READY, from connection_probe(). Once the routes are properly set, bumping the service to READY will trigger an update_nameservers() call, which should allow our DNS proxy to succesfully connect to the nameservers. Would you mind giving those 2 patches a try, please ? They should apply cleanly on top of the latest ConnMan git. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/
>From 48d443025dd243402250d05798f743eee78b6c40 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz <[email protected]> Date: Thu, 8 Jul 2010 19:08:14 +0200 Subject: [PATCH 1/2] Factorize host route setting routine --- include/inet.h | 3 +- src/connection.c | 8 +++--- src/inet.c | 60 ++++++----------------------------------------------- 3 files changed, 12 insertions(+), 59 deletions(-) diff --git a/include/inet.h b/include/inet.h index b5bf8a5..207410a 100644 --- a/include/inet.h +++ b/include/inet.h @@ -44,8 +44,7 @@ connman_bool_t connman_inet_is_cfg80211(int index); int connman_inet_set_address(int index, struct connman_ipaddress *ipaddress); int connman_inet_clear_address(int index); -int connman_inet_add_host_route_vpn(int index, const char *gateway, const char *host); -int connman_inet_add_host_route(int index, const char *host); +int connman_inet_add_host_route(int index, const char *host, const char *gateway); int connman_inet_del_host_route(int index, const char *host); int connman_inet_set_gateway_address(int index, const char *gateway); int connman_inet_clear_gateway_address(int index, const char *gateway); diff --git a/src/connection.c b/src/connection.c index 154076b..82648d6 100644 --- a/src/connection.c +++ b/src/connection.c @@ -157,7 +157,7 @@ static void set_default_gateway(struct gateway_data *data) goto done; } - connman_inet_add_host_route(element->index, data->gateway); + connman_inet_add_host_route(element->index, data->gateway, NULL); if (connman_inet_set_gateway_address(element->index, data->gateway) < 0) return; @@ -301,9 +301,9 @@ static int connection_probe(struct connman_element *element) } if (new_gateway->vpn == TRUE) { - connman_inet_add_host_route_vpn(active_gateway->index, - active_gateway->gateway, - new_gateway->gateway); + connman_inet_add_host_route(active_gateway->index, + new_gateway->gateway, + active_gateway->gateway); } if (new_gateway->order >= active_gateway->order) { diff --git a/src/inet.c b/src/inet.c index 980bd48..cfb7f92 100644 --- a/src/inet.c +++ b/src/inet.c @@ -612,58 +612,7 @@ int connman_inet_clear_address(int index) return 0; } -int connman_inet_add_host_route_vpn(int index, const char *gateway, const char *host) -{ - struct ifreq ifr; - struct rtentry rt; - struct sockaddr_in addr; - int sk, err; - - sk = socket(PF_INET, SOCK_DGRAM, 0); - if (sk < 0) - return -1; - - memset(&ifr, 0, sizeof(ifr)); - ifr.ifr_ifindex = index; - - if (ioctl(sk, SIOCGIFNAME, &ifr) < 0) { - close(sk); - return -1; - } - - DBG("ifname %s", ifr.ifr_name); - - memset(&rt, 0, sizeof(rt)); - rt.rt_flags = RTF_UP | RTF_HOST | RTF_GATEWAY; - - memset(&addr, 0, sizeof(addr)); - addr.sin_family = AF_INET; - addr.sin_addr.s_addr = inet_addr(host); - memcpy(&rt.rt_dst, &addr, sizeof(rt.rt_dst)); - - memset(&addr, 0, sizeof(addr)); - addr.sin_family = AF_INET; - addr.sin_addr.s_addr = inet_addr(gateway);; - memcpy(&rt.rt_gateway, &addr, sizeof(rt.rt_gateway)); - - memset(&addr, 0, sizeof(addr)); - addr.sin_family = AF_INET; - addr.sin_addr.s_addr = INADDR_ANY; - memcpy(&rt.rt_genmask, &addr, sizeof(rt.rt_genmask)); - - rt.rt_dev = ifr.ifr_name; - - err = ioctl(sk, SIOCADDRT, &rt); - if (err < 0) - connman_error("Adding host route failed (%s)", - strerror(errno)); - - close(sk); - - return err; -} - -int connman_inet_add_host_route(int index, const char *host) +int connman_inet_add_host_route(int index, const char *host, const char *gateway) { struct ifreq ifr; struct rtentry rt; @@ -686,6 +635,8 @@ int connman_inet_add_host_route(int index, const char *host) memset(&rt, 0, sizeof(rt)); rt.rt_flags = RTF_UP | RTF_HOST; + if (gateway != NULL) + rt.rt_flags |= RTF_GATEWAY; memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; @@ -694,7 +645,10 @@ int connman_inet_add_host_route(int index, const char *host) memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; - addr.sin_addr.s_addr = INADDR_ANY; + if (gateway != NULL) + addr.sin_addr.s_addr = inet_addr(gateway); + else + addr.sin_addr.s_addr = INADDR_ANY; memcpy(&rt.rt_gateway, &addr, sizeof(rt.rt_gateway)); memset(&addr, 0, sizeof(addr)); -- 1.7.1
>From c876b775f7e47907e68263c3b68a678a60ab8b86 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz <[email protected]> Date: Thu, 8 Jul 2010 19:27:40 +0200 Subject: [PATCH 2/2] Set DNS host routes before toggling the service READY state update_nameservers() is called when a service hits the READY state. The DNS proxy code will be able to connect to the added nameservers if the right host routes have been set. --- src/connection.c | 17 +++++++++++------ src/connman.h | 3 +++ src/service.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/connection.c b/src/connection.c index 82648d6..dcaa8fe 100644 --- a/src/connection.c +++ b/src/connection.c @@ -157,8 +157,6 @@ static void set_default_gateway(struct gateway_data *data) goto done; } - connman_inet_add_host_route(element->index, data->gateway, NULL); - if (connman_inet_set_gateway_address(element->index, data->gateway) < 0) return; @@ -276,14 +274,20 @@ static int connection_probe(struct connman_element *element) element->ipv4.gateway = g_strdup(gateway); } - service = __connman_element_get_service(element); - __connman_service_indicate_state(service, - CONNMAN_SERVICE_STATE_READY); - connman_element_set_enabled(element, TRUE); active_gateway = find_active_gateway(); new_gateway = add_gateway(element->index, gateway); + service = __connman_element_get_service(element); + + if (new_gateway) { + connman_inet_add_host_route(element->index, + new_gateway->gateway, NULL); + __connman_service_nameserver_add_routes(service, + new_gateway->gateway); + } + + __connman_service_indicate_state(service, CONNMAN_SERVICE_STATE_READY); if (service == NULL) { new_gateway->vpn = TRUE; @@ -325,6 +329,7 @@ static void connection_remove(struct connman_element *element) DBG("element %p name %s", element, element->name); service = __connman_element_get_service(element); + __connman_service_nameserver_del_routes(service); __connman_service_indicate_state(service, CONNMAN_SERVICE_STATE_DISCONNECT); diff --git a/src/connman.h b/src/connman.h index 3b8f7c2..7482d5b 100644 --- a/src/connman.h +++ b/src/connman.h @@ -448,6 +448,9 @@ void __connman_service_append_nameserver(struct connman_service *service, const char *nameserver); void __connman_service_remove_nameserver(struct connman_service *service, const char *nameserver); +void __connman_service_nameserver_add_routes(struct connman_service *service, + const char *gw); +void __connman_service_nameserver_del_routes(struct connman_service *service); unsigned long __connman_service_stats_get_rx_packets(struct connman_service *service); unsigned long __connman_service_stats_get_tx_packets(struct connman_service *service); diff --git a/src/service.c b/src/service.c index c76a9ee..7faa990 100644 --- a/src/service.c +++ b/src/service.c @@ -373,6 +373,41 @@ void __connman_service_remove_nameserver(struct connman_service *service, update_nameservers(service); } +void __connman_service_nameserver_add_routes(struct connman_service *service, + const char *gw) +{ + int index; + + index = connman_network_get_index(service->network); + + if (service->nameservers != NULL) { + int i; + + for (i = 0; service->nameservers[i]; i++) + connman_inet_add_host_route(index, + service->nameservers[i], gw); + } else if (service->nameserver != NULL) { + connman_inet_add_host_route(index, service->nameserver, gw); + } +} + +void __connman_service_nameserver_del_routes(struct connman_service *service) +{ + int index; + + index = connman_network_get_index(service->network); + + if (service->nameservers != NULL) { + int i; + + for (i = 0; service->nameservers[i]; i++) + connman_inet_del_host_route(index, + service->nameservers[i]); + } else if (service->nameserver != NULL) { + connman_inet_del_host_route(index, service->nameserver); + } +} + static void __connman_service_stats_start(struct connman_service *service) { DBG("service %p", service); -- 1.7.1
_______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
