Hi Samuel, Thanks for the feedback.
On Mon, Jul 05, 2010 at 02:39:11AM +0200, Samuel Ortiz wrote:
> On Thu, Jul 01, 2010 at 07:16:09PM -0400, Forest Bond wrote:
> > diff --git a/src/connection.c b/src/connection.c
> > index 154076b..5546a82 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -277,6 +277,7 @@ static int connection_probe(struct connman_element
> > *element)
> > }
> >
> > service = __connman_element_get_service(element);
> > + __connman_service_set_gateway(service, gateway);
> I don't particularily like having a service gateway pointer.
I don't like it either.
> Have you considered setting the nameserver host routes from
> __connman_service_append_nameserver(). All the callers know the underlying
> gateway (if any) and you could pass it as an argument to the above routine.
This seems right to me.
__connman_service_append_nameserver is called in two places. I need a little
help figuring out how to get the gateway in each case. Maybe you can confirm
these guesses:
ipv4_probe (ipv4.c):
connman_element_get_value(element, CONNMAN_PROPERTY_ID_IPV4_GATEWAY, NULL)
set_connected_manual (network.c):
__connman_ipconfig_get_gateway(connman_network_get_index(network))
Those seem most likely to me. I am just now grasping that
connman_element_get_value searches parent elements for a particular value. I
guess I don't really understand the purpose of network/ipconfig indexes.
If there was a diagram showing the relationship between the different
structures, that would be most helpful. It is difficult to visualize by reading
the source given the loose coupling provided by the element abstraction. Even a
text file explaining how the structures relate would make it easier to get
started.
> > diff --git a/src/service.c b/src/service.c
> > index fd694e8..fed9ff5 100644
> > --- a/src/service.c
> > +++ b/src/service.c
> > @@ -298,6 +299,39 @@ static connman_bool_t is_connected(const struct
> > connman_service *service)
> > return FALSE;
> > }
> >
> > +static int add_nameserver_host_route(struct connman_service *service,
> > + const char *nameserver)
> > +{
> > + int index;
> > +
> > + index = connman_network_get_index(service->network);
> > + if (service->gateway != NULL)
> > + return connman_inet_add_host_route_with_gateway(index,
> > + service->gateway, nameserver);
> > + else
> > + return connman_inet_add_host_route(index, nameserver);
> > +}
> This one would be nicer if integrated into a single inet.c host route setting
> routine, connman_inet_add_host_route() that would take an aditional gateway
> argument and set the rt_gateway address to ANY if the gateway argument is
> NULL.
> You would also then get rid of the existing connman_inet_add_host_route_vpn()
> one.
Sounds good to me.
> > +static int del_nameserver_host_route(struct connman_service *service,
> > + const char *nameserver)
> > +{
> > + int index;
> > + index = connman_network_get_index(service->network);
> > + return connman_inet_del_host_route(index, nameserver);
> > +}
> This one should be part of del_all_nameserver_host_routes().
Okay.
Thanks,
Forest
--
Forest Bond
http://www.alittletooquiet.net
http://www.pytagsfs.org
signature.asc
Description: Digital signature
_______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
