Hi Jukka, Thank you for the quick review and please find my answers inline. A new version of the patch is due, taking into account your suggestions.
Regards, Elena On Mon, May 14, 2012 at 2:21 PM, Jukka Rissanen < [email protected]> wrote: > 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 [ElenaT] ok > > > > 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. [ElenaT] ok > > > +/* 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 [ElenaT] ok > > > > 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. > [ElenaT] Agree. > > Also you need to ref the network as the network connection might be > disconnected while you have a refresh timeout going on. [ElenaT] Ok > > > + >> +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). [ElenaT] Will change the print message so that is logs only that this function has been entered, plus the input parameters > > > + >> + /* 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? [ElenaT] These days there is an ongoing discussion on the ipv6 IETF mail list about RDNSS lifetimes - roughly the same feature I implemented with this patch. The mail discussion can be checked here: http://www.ietf.org/mail-archive/web/ipv6/current/msg15820.html While this was not yet formalized as part of RFC, it seems that today there is a draft in progress. According to the draft, RS should be sent when a threshold of lifetime is reached, and then resend RS (a number of times) at specified time intervals. At the moment, the draft does not enforce / suggests values for all these variables (lifetime threshold, RS resend count, RS resend interval). The changes I've done in this patch implements the RS refresh mechanism, allowing it to be adjusted very easy with the right variables when the RFC will be finalized. + 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 [ElenaT] > > > +/* 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 [ElenaT] > > > >> 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 [ElenaT] ok > > > + 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. [ElenaT] ok > > > + " 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. > [ElenaT] ok > > + refresh_rs_ipv6(__connman_** >> service_get_network(service), >> + index); >> > > Function naming convention. You should use __connman prefix here. [ElenaT] ok > > > + } >> + } >> + 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" [ElenaT] ok > > > + " interface %s domain %s server %s >> lifetime threshold %d", >> > > Line len > 80 [ElenaT] ok > > > + 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. > [ElenaT] What I tried with floor and ceil is to split the lifetime into two intervals without losing additional 1second with conversion from floating point to integer. RESOLVER_LIFETIME_REFRESH_THRESHOLD is floating point here. > > Also extra linefeed here [ElenaT] ok > > > + DBG("RDNSS start lifetime threshold timer." >> + " interface %s domain %s server %s >> lifetime threshold %d", >> > > Line len > 80 [ElenaT] ok > > > + 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. > [ElenaT] The current Connman code handles the case of extending RDNSS lifetime the same way as adding a new RDNSS lifetime. Meaning it will first remove the RDNSS entry(which is not yet expired) then add a new entry for the new lifetime. This is the reason the code is similar in the two cases. Perhaps the behavior of extending lifetime could be changed in the future so that existing lifetime is indeed extended. With current code, RDNSS entries may expire sooner than supposed to, because lifetime is not extended. > > > return 0; >> } >> >> > > Cheers, > Jukka > _______________________________________________ connman mailing list [email protected] http://lists.connman.net/listinfo/connman
