Hi Elena,
On 05/11/2012 02:56 PM, Elena Tebesoi wrote:
Implemented feature from RFC 6106 section '5.1. Recursive DNS Server Option':
"Lifetime 32-bit unsigned integer.
...
Hosts MAY send a Router Solicitation to ensure the RDNSS information is
fresh before the interval expires."
Host will send RS when a certain threshold of RDNSS lifetime is reached.
Values which can be adjusted:
- lifetime threshold - currently configured with a value of 80% from lifetime
- number of retries in case RA is not received - currently set to 0
- time between retries, in case RA is not received - currently set to 3 seconds
---
include/network.h | 2 +
src/network.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
src/resolver.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 108 insertions(+), 4 deletions(-)
diff --git a/include/network.h b/include/network.h
index 12b0621..d602683 100644
--- a/include/network.h
+++ b/include/network.h
@@ -147,6 +147,8 @@ void connman_network_set_data(struct connman_network
*network, void *data);
void connman_network_update(struct connman_network *network);
+int refresh_rs_ipv6(struct connman_network *network, int index);
+
Because the refresh_rs_ipv6() is to be called inside core (no plugin
involved), then follow the naming convention (use __connman_* prefix)
and place the prototype into connman.h
struct connman_network_driver {
const char *name;
enum connman_network_type type;
diff --git a/src/network.c b/src/network.c
index 10f1bf3..311b3b8 100644
--- a/src/network.c
+++ b/src/network.c
@@ -28,8 +28,17 @@
#include "connman.h"
+/* How many times to retry sending RS with the purpose of
+ * refreshing RDNSS entries before they actually expire.
+ * With a value of 0, one RS will be sent, with no retries. */
+#define RS_REFRESH_RETRIES 0
Use commenting style like this
/*
* How many times to retry ...
*/
Put extra line feed here.
+/* Value in seconds to wait for RA after RS was sent.
+ * After this time elapsed, we can send another RS. */
+#define RS_REFRESH_TIMEOUT 3
+
Commenting style issue here
static GSList *network_list = NULL;
static GSList *driver_list = NULL;
+static connman_bool_t refresh_rs_in_progress = FALSE;
struct connman_network {
int refcount;
@@ -1113,6 +1122,51 @@ static void check_dhcpv6(struct nd_router_advert *reply,
connman_network_unref(network);
}
+static void receive_refresh_rs_reply(struct nd_router_advert *reply,
+ unsigned int length, void *user_data)
+{
+ struct connman_network *network = user_data;
+
+ DBG("reply %p", reply);
+
+ if (reply == NULL) {
+ /*
+ * Router solicitation message seem to get lost easily so
+ * try to send it again.
+ */
+ if (network->router_solicit_count> 0) {
+ DBG("re-send router solicitation %d",
+ network->router_solicit_count);
+ network->router_solicit_count--;
+ __connman_inet_ipv6_send_rs(network->index, 1,
+ receive_refresh_rs_reply, network);
+ return;
+ }
+ }
+
+ network->router_solicit_count = 0;
+ refresh_rs_in_progress = FALSE;
+}
You should probably use different counter than router_solicit_count
here. You would not need separate refresh_rs_in_progress flag because
your counter could tell if you have refresh going on.
Also you need to ref the network as the network connection might be
disconnected while you have a refresh timeout going on.
+
+int refresh_rs_ipv6(struct connman_network *network, int index)
+{
+ int ret = 0;
+
+ DBG("Send RS before RDNSS entries expire");
Do not print these debug prints if you are not going to send anything
(refresh_rs_in_progress == TRUE).
+
+ /* Send only one RS for all RDNSS entries which are about to expire */
+ if (refresh_rs_in_progress == TRUE)
+ return 0;
Better would be to print something here (if really necessary)
+ DBG("RS refresh network %p index %d", network, index);
(just an example)
+
+ refresh_rs_in_progress = TRUE;
+
+ /* Just send one RS message, without retrying */
+ network->router_solicit_count = RS_REFRESH_RETRIES;
+ ret = __connman_inet_ipv6_send_rs(index, RS_REFRESH_TIMEOUT,
+ receive_refresh_rs_reply, network);
If you are not interested to receiving RA or retrying, just leave the
callback and user data out (use NULL). Then you will not need to keep
track of network pointer.
If you are only sending one RS without retries, why do you bother
setting up the router_solicit_count variable in the first place?
+ return ret;
+}
+
static void autoconf_ipv6_set(struct connman_network *network)
{
struct connman_service *service;
diff --git a/src/resolver.c b/src/resolver.c
index 9656838..3a0ea3d 100644
--- a/src/resolver.c
+++ b/src/resolver.c
@@ -31,16 +31,21 @@
#include<string.h>
#include<sys/stat.h>
#include<resolv.h>
+#include<math.h>
#include "connman.h"
#define RESOLVER_FLAG_PUBLIC (1<< 0)
Put extra line feed here
+/* Threshold for RDNSS lifetime. Will be used to trigger RS
+ * before RDNSS entries actually expire */
+#define RESOLVER_LIFETIME_REFRESH_THRESHOLD (0.8)
Fix commenting style
struct entry_data {
char *interface;
char *domain;
char *server;
unsigned int flags;
+ unsigned int lifetime;
guint timeout;
};
@@ -253,11 +258,41 @@ static gboolean resolver_expire_cb(gpointer user_data)
return FALSE;
}
+static gboolean resolver_refresh_cb(gpointer user_data)
+{
+ struct entry_data *entry = user_data;
+ int index;
+ unsigned int interval;
+ struct connman_service *service = NULL;
+
+ interval = ceil(entry->lifetime *
+ (1 - RESOLVER_LIFETIME_REFRESH_THRESHOLD));
Extra line feed here
+ DBG("RDNSS start lifetime timer remainder from threshold."
Please make this first sentence shorter, it is just too long (especially
with the remaining string) and does not give any extra information.
+ " interface %s domain %s server %s remaining lifetime %d",
+ entry->interface, entry->domain, entry->server, interval);
+
+ entry->timeout = g_timeout_add_seconds(interval,
+ resolver_expire_cb, entry);
+
+ index = connman_inet_ifindex(entry->interface);
+ if (index>= 0) {
+ service = __connman_service_lookup_from_index(index);
+ if (service != NULL) {
+ /* Send Router Solicitation to refresh RDNSS entries
+ * before their lifetime expires */
Fix commenting style here.
+ refresh_rs_ipv6(__connman_service_get_network(service),
+ index);
Function naming convention. You should use __connman prefix here.
+ }
+ }
+ return FALSE;
+}
+
static int append_resolver(const char *interface, const char *domain,
const char *server, unsigned int lifetime,
unsigned int flags)
{
struct entry_data *entry;
+ unsigned int interval;
DBG("interface %s domain %s server %s lifetime %d flags %d",
interface, domain, server, lifetime, flags);
@@ -273,10 +308,16 @@ static int append_resolver(const char *interface, const
char *domain,
entry->domain = g_strdup(domain);
entry->server = g_strdup(server);
entry->flags = flags;
+ entry->lifetime = lifetime;
if (lifetime) {
int index;
- entry->timeout = g_timeout_add_seconds(lifetime,
- resolver_expire_cb, entry);
+ interval = floor(lifetime *
+ RESOLVER_LIFETIME_REFRESH_THRESHOLD);
+ DBG("RDNSS start lifetime threshold timer."
What about just
DBG("RDNSS start"
+ " interface %s domain %s server %s lifetime
threshold %d",
Line len > 80
+ interface, domain, server, interval);
+ entry->timeout = g_timeout_add_seconds(interval,
+ resolver_refresh_cb, entry);
/*
* We update the service only for those nameservers
@@ -350,6 +391,7 @@ int connman_resolver_append_lifetime(const char *interface,
const char *domain,
const char *server, unsigned int lifetime)
{
GSList *list;
+ unsigned int interval;
DBG("interface %s domain %s server %s lifetime %d",
interface, domain, server, lifetime);
@@ -373,8 +415,14 @@ int connman_resolver_append_lifetime(const char
*interface, const char *domain,
return 0;
}
- entry->timeout = g_timeout_add_seconds(lifetime,
- resolver_expire_cb, entry);
+ interval = floor(lifetime *
+ RESOLVER_LIFETIME_REFRESH_THRESHOLD);
Do we really need to use floor() here? What about just
interval = lifetime * RESOLVER_LIFETIME_REFRESH_THRESHOLD;
The lifetime > 0 so we cannot have negative interval anyway.
Also extra linefeed here
+ DBG("RDNSS start lifetime threshold timer."
+ " interface %s domain %s server %s lifetime
threshold %d",
Line len > 80
+ interface, domain, server, interval);
+
+ entry->timeout = g_timeout_add_seconds(interval,
+ resolver_refresh_cb, entry);
You are adding the timer here and also in append_resolver(). Could it be
possible to refactor the code (use common function or similar), I am
just guessing here.
return 0;
}
Cheers,
Jukka
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
