Discussion:
[PATCH] IPv6 Timeserver for NTP
(too old to reply)
Naveen Singh
2015-09-24 04:22:01 UTC
Permalink
From: Naveen Singh <***@nestlabs.com>

Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 113 insertions(+), 31 deletions(-)

diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..5c135ce 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>

#include <glib.h>

@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;

static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;

-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout, int family);

static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1, timeserver_addr.ss_family);

return FALSE;
}

-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout, int family)
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;

/*
* At some point, we could specify the actual system precision with:
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;

- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }

gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);

len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;

- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT, timeserver_addr.ss_family);

return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
if (len < 0)
return TRUE;

- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in *)&sender_addr)->sin_addr.s_addr)
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
+ ((struct sockaddr_in6 *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
+ 16) != 0)
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }

tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -419,40 +451,58 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
return TRUE;
}

-static void start_ntp(char *server)
+static void start_ntp(char *server, int family)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;

if (!server)
return;

- DBG("server %s", server);
+ DBG("server %s family %d", server, family);

if (channel_watch > 0)
goto send;

- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}

- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
-
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
- connman_error("Failed to bind time server socket");
- close(transmit_fd);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ } else {
+ connman_error("Family not correct");
return;
}

- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
- connman_error("Failed to set type of service option");
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
+ connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
+ connman_error("Failed to set type of service option");
+ close(transmit_fd);
+ return;
+ }
+ } else if (family == AF_INET6) {
+ //TO Do
+ }

if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
sizeof(timestamp)) < 0) {
@@ -479,12 +529,16 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);

send:
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}

int __connman_ntp_start(char *server)
{
DBG("%s", server);
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
+ int ret;

if (!server)
return -EINVAL;
@@ -492,10 +546,38 @@ int __connman_ntp_start(char *server)
if (timeserver)
g_free(timeserver);

+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get the server info");
+ return -1;
+ }
+
+ family = info->ai_family;
+ if (family == AF_INET) {
+ if (inet_pton(family, server, &(((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr)) == 0) {
+ connman_error("cannot convert ip address string");
+ return -1;
+ }
+ } else if (family == AF_INET6) {
+ if (inet_pton(family, server, ((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
+ connman_error("cannot convert ipv6 address string");
+ return -1;
+ }
+ } else {
+ connman_error("Neither IPv4 nor IPv6 type");
+ return -1;
+ }
+
+ timeserver_addr.ss_family = family;
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);

- start_ntp(timeserver);
+ freeaddrinfo(info);
+
+ start_ntp(timeserver, family);

return 0;
}
--
2.5.3
Patrik Flykt
2015-09-24 08:14:24 UTC
Permalink
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 113 insertions(+), 31 deletions(-)
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..5c135ce 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout, int family);
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1, timeserver_addr.ss_family);
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout, int family)
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT, timeserver_addr.ss_family);
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in *)&sender_addr)->sin_addr.s_addr)
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
+ ((struct sockaddr_in6 *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
+ 16) != 0)
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -419,40 +451,58 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
return TRUE;
}
-static void start_ntp(char *server)
+static void start_ntp(char *server, int family)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
if (!server)
return;
- DBG("server %s", server);
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
-
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
- connman_error("Failed to bind time server socket");
- close(transmit_fd);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ } else {
+ connman_error("Family not correct");
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
- connman_error("Failed to set type of service option");
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
+ connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
+ connman_error("Failed to set type of service option");
+ close(transmit_fd);
+ return;
+ }
+ } else if (family == AF_INET6) {
+ //TO Do
+ }
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
sizeof(timestamp)) < 0) {
@@ -479,12 +529,16 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
{
DBG("%s", server);
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
+ int ret;
if (!server)
return -EINVAL;
@@ -492,10 +546,38 @@ int __connman_ntp_start(char *server)
if (timeserver)
g_free(timeserver);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
This functionality already exists in the form of
_connman_inet_check_ipaddress(). It takes the server as an argument and
returns the matching AF_ value if it's an IP address. With this in mind,
the check itself should be done in start_ntp() to reduce extra
parameters carried between the functions. That way it shrinks down the
patch to the bare minimum changes needed.
Post by Naveen Singh
+
+ if (ret) {
+ connman_error("cannot get the server info");
+ return -1;
+ }
+
+ family = info->ai_family;
+ if (family == AF_INET) {
+ if (inet_pton(family, server, &(((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr)) == 0) {
+ connman_error("cannot convert ip address string");
+ return -1;
+ }
+ } else if (family == AF_INET6) {
+ if (inet_pton(family, server, ((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
+ connman_error("cannot convert ipv6 address string");
+ return -1;
+ }
+ } else {
+ connman_error("Neither IPv4 nor IPv6 type");
+ return -1;
This check has already happened in src/service.c, set_property() and
thus need not be re-done here. It is not done in src/clock.c
set_property(), neither for the main.conf FallbackTimeservers, so those
parts need a new patch each.
Post by Naveen Singh
+ }
+
+ timeserver_addr.ss_family = family;
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
- start_ntp(timeserver);
+ freeaddrinfo(info);
+
+ start_ntp(timeserver, family);
return 0;
}
Cheers,

Patrik
Patrik Flykt
2015-09-24 10:02:44 UTC
Permalink
Post by Patrik Flykt
+
+ if (ret) {
+ connman_error("cannot get the server info");
+ return -1;
+ }
+
+ family = info->ai_family;
+ if (family == AF_INET) {
+ if (inet_pton(family, server, &(((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr)) == 0) {
+ connman_error("cannot convert ip address string");
+ return -1;
+ }
+ } else if (family == AF_INET6) {
+ if (inet_pton(family, server, ((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
+ connman_error("cannot convert ipv6 address string");
+ return -1;
+ }
+ } else {
+ connman_error("Neither IPv4 nor IPv6 type");
+ return -1;
This check has already happened in src/service.c, set_property() and
thus need not be re-done here. It is not done in src/clock.c
set_property(), neither for the main.conf FallbackTimeservers, so those
parts need a new patch each.
Let me correct my comment above. Timeservers can also be specified using
host names, so no IP address checks need to be done in the above places.

src/timeservers.c has the code to figure out whether name resolution is
needed or whether the given name is an IP address. So
__connman_ntp_start() can therefore always expect to get either an IPv4
or an IPv6 address.

Cheers,

Patrik
Naveen Singh
2015-09-24 22:38:47 UTC
Permalink
Post by Naveen Singh
Post by Patrik Flykt
+
+ if (ret) {
+ connman_error("cannot get the server info");
+ return -1;
+ }
+
+ family = info->ai_family;
+ if (family == AF_INET) {
+ if (inet_pton(family, server, &(((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Patrik Flykt
+ connman_error("cannot convert ip address
string");
Post by Patrik Flykt
+ return -1;
+ }
+ } else if (family == AF_INET6) {
+ if (inet_pton(family, server, ((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Patrik Flykt
+ connman_error("cannot convert ipv6 address
string");
Post by Patrik Flykt
+ return -1;
+ }
+ } else {
+ connman_error("Neither IPv4 nor IPv6 type");
+ return -1;
This check has already happened in src/service.c, set_property() and
thus need not be re-done here. It is not done in src/clock.c
set_property(), neither for the main.conf FallbackTimeservers, so those
parts need a new patch each.
Let me correct my comment above. Timeservers can also be specified using
host names, so no IP address checks need to be done in the above places.
src/timeservers.c has the code to figure out whether name resolution is
needed or whether the given name is an IP address. So
__connman_ntp_start() can therefore always expect to get either an IPv4
or an IPv6 address.
Isn't the IPv4 or IPv6 address passed as an argument to __connman_ntp_start
a character array? We would still need to convert this character array to
the IP addresses and for that we would need to know the family. Do you
think there is any other way to do this without using inet_pton (which
requires the family knowledge). Let me know.
Post by Naveen Singh
Cheers,
Patrik
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman
Patrik Flykt
2015-09-25 07:15:47 UTC
Permalink
Post by Naveen Singh
Isn't the IPv4 or IPv6 address passed as an argument to
__connman_ntp_start a character array? We would still need to convert
this character array to the IP addresses and for that we would need to
know the family. Do you think there is any other way to do this
without using inet_pton (which requires the family knowledge). Let me
know.
Since the string is always an address, a shortcut would be to use
getaddrinfo() directly with hints.ai_flags = AI_NUMERICHOST. This is
what connman_inet_check_ipaddress() does, but in start_ntp() the address
is also needed, not only the address family.

The point is that the conversion from string to addres and address
family need to be done only when needed to make the patch smaller and
less intrusive.

Cheers,

Patrik
Naveen Singh
2015-09-30 04:50:18 UTC
Permalink
Sorry for a delayed reply. Somehow this email went to a different folder.
Post by Patrik Flykt
Post by Naveen Singh
Isn't the IPv4 or IPv6 address passed as an argument to
__connman_ntp_start a character array? We would still need to convert
this character array to the IP addresses and for that we would need to
know the family. Do you think there is any other way to do this
without using inet_pton (which requires the family knowledge). Let me
know.
Since the string is always an address, a shortcut would be to use
getaddrinfo() directly with hints.ai_flags = AI_NUMERICHOST. This is
what connman_inet_check_ipaddress() does, but in start_ntp() the address
is also needed, not only the address family.
This is what I do but in __connman_ntp_start.
Post by Patrik Flykt
The point is that the conversion from string to addres and address
family need to be done only when needed to make the patch smaller and
less intrusive.
I got your point. So to keep the patch less intrusive:

1. Do not modify __connman_ntp_start instead modify ntp_start
2. In start_ntp find out family by calling getaddrinfo and use inet_pton to
store the correct IP address in timeserver_addr.

I will make the changes and provide you the patch tomorrow after testing it
on my setup.
Post by Patrik Flykt
Cheers,
Patrik
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman
Naveen Singh
2015-09-24 22:36:01 UTC
Permalink
Hi,
Post by Naveen Singh
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 144
++++++++++++++++++++++++++++++++++++++++++++++++--------------
Post by Naveen Singh
1 file changed, 113 insertions(+), 31 deletions(-)
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..5c135ce 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
Post by Naveen Singh
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family);
Post by Naveen Singh
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family)
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
* At some point, we could specify the actual system precision
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char
*server, uint32_t timeout)
Post by Naveen Singh
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server,
uint32_t timeout)
Post by Naveen Singh
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in
*)&sender_addr)->sin_addr.s_addr)
Post by Naveen Singh
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ ((struct sockaddr_in6
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ 16) != 0)
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -419,40 +451,58 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
return TRUE;
}
-static void start_ntp(char *server)
+static void start_ntp(char *server, int family)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
if (!server)
return;
- DBG("server %s", server);
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
-
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) <
0) {
Post by Naveen Singh
- connman_error("Failed to bind time server socket");
- close(transmit_fd);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ } else {
+ connman_error("Family not correct");
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos))
< 0) {
Post by Naveen Singh
- connman_error("Failed to set type of service option");
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
+ connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos,
sizeof(tos)) < 0) {
Post by Naveen Singh
+ connman_error("Failed to set type of service
option");
Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
+ } else if (family == AF_INET6) {
+ //TO Do
+ }
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
sizeof(timestamp)) < 0) {
@@ -479,12 +529,16 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
{
DBG("%s", server);
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
+ int ret;
if (!server)
return -EINVAL;
@@ -492,10 +546,38 @@ int __connman_ntp_start(char *server)
if (timeserver)
g_free(timeserver);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
This functionality already exists in the form of
_connman_inet_check_ipaddress(). It takes the server as an argument and
returns the matching AF_ value if it's an IP address. With this in mind,
the check itself should be done in start_ntp() to reduce extra
parameters carried between the functions. That way it shrinks down the
patch to the bare minimum changes needed.
The only reason i was calling this function here is to get family of the IP
address (as argument passed to this function is just a character string).
This family is needed for inet_pton so that we can convert the charater
string into the appropriate IP address struct and store it in
timeserver_addr. Since we got the family here I just passed it to start_ntp
so we do not have to do this again.
Post by Naveen Singh
Post by Naveen Singh
+
+ if (ret) {
+ connman_error("cannot get the server info");
+ return -1;
+ }
+
+ family = info->ai_family;
+ if (family == AF_INET) {
+ if (inet_pton(family, server, &(((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Naveen Singh
+ connman_error("cannot convert ip address string");
+ return -1;
+ }
+ } else if (family == AF_INET6) {
+ if (inet_pton(family, server, ((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Naveen Singh
+ connman_error("cannot convert ipv6 address
string");
Post by Naveen Singh
+ return -1;
+ }
+ } else {
+ connman_error("Neither IPv4 nor IPv6 type");
+ return -1;
This check has already happened in src/service.c, set_property() and
thus need not be re-done here. It is not done in src/clock.c
set_property(), neither for the main.conf FallbackTimeservers, so those
parts need a new patch each.
This check here is to just store the appropriate IP address into
timerserver_addr and knowing family helps us in typecasting timeserver_addr
to appropriate structure.
Post by Naveen Singh
Post by Naveen Singh
+ }
+
+ timeserver_addr.ss_family = family;
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
- start_ntp(timeserver);
+ freeaddrinfo(info);
+
+ start_ntp(timeserver, family);
return 0;
}
Cheers,
Patrik
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman
Naveen Singh
2015-10-01 05:08:53 UTC
Permalink
From: Naveen Singh <***@nestlabs.com>

Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 101 insertions(+), 28 deletions(-)

diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>

#include <glib.h>

@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;

static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;

-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout, int family);

static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1, timeserver_addr.ss_family);

return FALSE;
}

-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout, int family)
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;

/*
* At some point, we could specify the actual system precision with:
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;

- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }

gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);

len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;

- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT, timeserver_addr.ss_family);

return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
if (len < 0)
return TRUE;

- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in *)&sender_addr)->sin_addr.s_addr)
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
+ ((struct sockaddr_in6 *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
+ 16) != 0)
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }

tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;

if (!server)
return;

- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
+
+ DBG("server %s family %d", server, family);

if (channel_watch > 0)
goto send;

- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}

- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr)) == 0) {
+ DBG("cannot convert ipv4 address string");
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
+ DBG("cannot convert ipv6 address string");
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }

- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}

- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
- connman_error("Failed to set type of service option");
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
+ connman_error("Failed to set type of service option");
+ close(transmit_fd);
+ return;
+ }
}

if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);

send:
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}

int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char *server)
g_free(timeserver);

timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);

start_ntp(timeserver);
--
2.5.3
Patrik Flykt
2015-10-01 13:26:43 UTC
Permalink
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 101 insertions(+), 28 deletions(-)
This was patch version 2, so the next one is version 3. Add the version
to the Subject: line as [PATCH v3], this can be done with
'git ... --subject-prefix="PATCH v3" ...'.
Post by Naveen Singh
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout, int family);
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1, timeserver_addr.ss_family);
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout, int family)
Instead of supplying a char *server, why don't we give a struct
sockaddr* instead? What's the purpose of carrying the server
"name" (nah, the IP address as a string), if it is already resolved in
the calling function? BTW, it is available as timeserver_addr, so do we
really need any of this information here?
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
The above is unnecessary. You already have timeserver_addr set or the
function gets a sockaddr * to use.
Post by Naveen Singh
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server, uint32_t timeout)
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
Use the stored timeserver_addr or supplied sockaddr * family information
to set the size to either sizeof(struct sockaddr_in) or sizeof(struct
sockaddr_in6).
Post by Naveen Singh
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT, timeserver_addr.ss_family);
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in *)&sender_addr)->sin_addr.s_addr)
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
+ ((struct sockaddr_in6 *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
+ 16) != 0)
Use memcmp for both of these. First check address families, if they
match the size can be deduced (for example as above) and have only a
single memcmp to do the matching.
Post by Naveen Singh
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean received_data(GIOChannel *channel, GIOCondition condition,
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;
if (!server)
return;
- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
Before throwing away info, the address is already available as
->sin6_addr or ->sin_addr. See man getaddrinfo. Store the address and
the address family in timeserver_addr.
Post by Naveen Singh
+
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct sockaddr_in *)&timeserver_addr)->sin_addr.s_addr)) == 0) {
+ DBG("cannot convert ipv4 address string");
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
+ DBG("cannot convert ipv6 address string");
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }
No need for the above, look into the 'info' supplied to getaddrinfo()
above.
Post by Naveen Singh
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
- connman_error("Failed to set type of service option");
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) {
+ connman_error("Failed to set type of service option");
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char *server)
g_free(timeserver);
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
start_ntp(timeserver);
Patrik
Naveen Singh
2015-10-01 19:22:25 UTC
Permalink
Post by Naveen Singh
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 129
++++++++++++++++++++++++++++++++++++++++++++++++--------------
Post by Naveen Singh
1 file changed, 101 insertions(+), 28 deletions(-)
This was patch version 2, so the next one is version 3. Add the version
to the Subject: line as [PATCH v3], this can be done with
'git ... --subject-prefix="PATCH v3" ...'.
Post by Naveen Singh
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
Post by Naveen Singh
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family);
Post by Naveen Singh
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family)
Instead of supplying a char *server, why don't we give a struct
sockaddr* instead? What's the purpose of carrying the server
"name" (nah, the IP address as a string), if it is already resolved in
the calling function? BTW, it is available as timeserver_addr, so do we
really need any of this information here?
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
* At some point, we could specify the actual system precision
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char
*server, uint32_t timeout)
Post by Naveen Singh
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
The above is unnecessary. You already have timeserver_addr set or the
function gets a sockaddr * to use.
Post by Naveen Singh
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server,
uint32_t timeout)
Post by Naveen Singh
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
Use the stored timeserver_addr or supplied sockaddr * family information
to set the size to either sizeof(struct sockaddr_in) or sizeof(struct
sockaddr_in6).
Post by Naveen Singh
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in
*)&sender_addr)->sin_addr.s_addr)
Post by Naveen Singh
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ ((struct sockaddr_in6
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ 16) != 0)
Use memcmp for both of these. First check address families, if they
match the size can be deduced (for example as above) and have only a
single memcmp to do the matching.
Post by Naveen Singh
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;
if (!server)
return;
- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
Before throwing away info, the address is already available as
->sin6_addr or ->sin_addr. See man getaddrinfo. Store the address and
the address family in timeserver_addr.
Post by Naveen Singh
+
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv4 address string");
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv6 address string");
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }
No need for the above, look into the 'info' supplied to getaddrinfo()
above.
Post by Naveen Singh
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) <
0) {
Post by Naveen Singh
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos))
< 0) {
Post by Naveen Singh
- connman_error("Failed to set type of service option");
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos,
sizeof(tos)) < 0) {
Post by Naveen Singh
+ connman_error("Failed to set type of service
option");
Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char *server)
g_free(timeserver);
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
start_ntp(timeserver);
Patrik
I really did not want to diverge too much from the original code (in terms
of changing the function signatures and logic) and that is why i did not
want to pass the address directly. These all are valid sugestions and I
will send you the version 3 patch after doing the testing.
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman
Naveen Singh
2015-10-02 06:00:13 UTC
Permalink
Post by Naveen Singh
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 129
++++++++++++++++++++++++++++++++++++++++++++++++--------------
Post by Naveen Singh
1 file changed, 101 insertions(+), 28 deletions(-)
This was patch version 2, so the next one is version 3. Add the version
to the Subject: line as [PATCH v3], this can be done with
'git ... --subject-prefix="PATCH v3" ...'.
Post by Naveen Singh
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
Post by Naveen Singh
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family);
Post by Naveen Singh
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family)
Instead of supplying a char *server, why don't we give a struct
sockaddr* instead? What's the purpose of carrying the server
"name" (nah, the IP address as a string), if it is already resolved in
the calling function? BTW, it is available as timeserver_addr, so do we
really need any of this information here?
Yes it makes sense but I am not sure sockaddr * is the one we should us. It
will not be able to hold IPv6 address which is 16 bytes. So I guess
sockaddr_storage* would be the right thing. Do you agree? If you look i
changed type of timeserver_addr to sockaddr_storage. Do you agree with this?

/* Structure describing a generic socket address. */
struct sockaddr
{
__SOCKADDR_COMMON (sa_); /* Common data: address family and length.
*/
char sa_data[14]; /* Address data. */
};
Post by Naveen Singh
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
* At some point, we could specify the actual system precision
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char
*server, uint32_t timeout)
Post by Naveen Singh
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
The above is unnecessary. You already have timeserver_addr set or the
function gets a sockaddr * to use.
Agreed. If we pass sockaddr_storage*.
Post by Naveen Singh
Post by Naveen Singh
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server,
uint32_t timeout)
Post by Naveen Singh
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
Use the stored timeserver_addr or supplied sockaddr * family information
to set the size to either sizeof(struct sockaddr_in) or sizeof(struct
sockaddr_in6).
I guess this is what i am doing. But will double confirm.
Post by Naveen Singh
Post by Naveen Singh
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in
*)&sender_addr)->sin_addr.s_addr)
Post by Naveen Singh
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ ((struct sockaddr_in6
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ 16) != 0)
Use memcmp for both of these. First check address families, if they
match the size can be deduced (for example as above) and have only a
single memcmp to do the matching.
Agreed.
Post by Naveen Singh
Post by Naveen Singh
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;
if (!server)
return;
- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
Before throwing away info, the address is already available as
->sin6_addr or ->sin_addr. See man getaddrinfo. Store the address and
the address family in timeserver_addr.
I guess this may not be correct as getaddrinfo gives us a pointer to
sockaddr which may not be able to contain the 16 byte IPv6 address. Do you
agree? I guess using inet_pton is the best way.
Post by Naveen Singh
Post by Naveen Singh
+
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv4 address string");
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv6 address string");
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }
No need for the above, look into the 'info' supplied to getaddrinfo()
above.
Post by Naveen Singh
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) <
0) {
Post by Naveen Singh
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos))
< 0) {
Post by Naveen Singh
- connman_error("Failed to set type of service option");
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos,
sizeof(tos)) < 0) {
Post by Naveen Singh
+ connman_error("Failed to set type of service
option");
Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char *server)
g_free(timeserver);
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
start_ntp(timeserver);
Patrik
Regards
Naveen
Post by Naveen Singh
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman
Jukka Rissanen
2015-10-02 07:29:27 UTC
Permalink
Hi Naveen,
Post by Naveen Singh
Post by Naveen Singh
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the socket
operation would fail with error as Permission denied (13). This change in
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 129
++++++++++++++++++++++++++++++++++++++++++++++++--------------
Post by Naveen Singh
1 file changed, 101 insertions(+), 28 deletions(-)
This was patch version 2, so the next one is version 3. Add the version
to the Subject: line as [PATCH v3], this can be done with
'git ... --subject-prefix="PATCH v3" ...'.
Post by Naveen Singh
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
Post by Naveen Singh
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t timeout);
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family);
Post by Naveen Singh
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer user_data)
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t timeout)
+static void send_packet(int fd, const char *server, uint32_t timeout,
int family)
Instead of supplying a char *server, why don't we give a struct
sockaddr* instead? What's the purpose of carrying the server
"name" (nah, the IP address as a string), if it is already resolved in
the calling function? BTW, it is available as timeserver_addr, so do we
really need any of this information here?
Yes it makes sense but I am not sure sockaddr * is the one we should us. It
will not be able to hold IPv6 address which is 16 bytes. So I guess
sockaddr_storage* would be the right thing. Do you agree? If you look i
changed type of timeserver_addr to sockaddr_storage. Do you agree with this?
/* Structure describing a generic socket address. */
struct sockaddr
{
__SOCKADDR_COMMON (sa_); /* Common data: address family and length.
*/
char sa_data[14]; /* Address data. */
};
You should never instantiate (== have a variable "struct sockaddr
myaddr") because there is not enough space as you noticed. You either
have a "struct sockaddr_in myaddr" or "struct sockaddr_in6 myaddr" and
then you just cast "struct sockaddr *" to correct variable.
There is no need to use sockaddr_storage here.
Post by Naveen Singh
Post by Naveen Singh
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
* At some point, we could specify the actual system precision
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char
*server, uint32_t timeout)
Post by Naveen Singh
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
The above is unnecessary. You already have timeserver_addr set or the
function gets a sockaddr * to use.
Agreed. If we pass sockaddr_storage*.
No, sockaddr * is enough.
Post by Naveen Singh
Post by Naveen Singh
Post by Naveen Singh
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char *server,
uint32_t timeout)
Post by Naveen Singh
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
Use the stored timeserver_addr or supplied sockaddr * family information
to set the size to either sizeof(struct sockaddr_in) or sizeof(struct
sockaddr_in6).
I guess this is what i am doing. But will double confirm.
Post by Naveen Singh
Post by Naveen Singh
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
gpointer user_data)
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr)
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in
*)&sender_addr)->sin_addr.s_addr)
Post by Naveen Singh
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ ((struct sockaddr_in6
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ 16) != 0)
Use memcmp for both of these. First check address families, if they
match the size can be deduced (for example as above) and have only a
single memcmp to do the matching.
Agreed.
Post by Naveen Singh
Post by Naveen Singh
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean received_data(GIOChannel *channel,
GIOCondition condition,
Post by Naveen Singh
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;
if (!server)
return;
- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
Before throwing away info, the address is already available as
->sin6_addr or ->sin_addr. See man getaddrinfo. Store the address and
the address family in timeserver_addr.
I guess this may not be correct as getaddrinfo gives us a pointer to
sockaddr which may not be able to contain the 16 byte IPv6 address. Do you
agree? I guess using inet_pton is the best way.
Post by Naveen Singh
Post by Naveen Singh
+
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv4 address string");
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv6 address string");
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }
No need for the above, look into the 'info' supplied to getaddrinfo()
above.
Post by Naveen Singh
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) <
0) {
Post by Naveen Singh
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos))
< 0) {
Post by Naveen Singh
- connman_error("Failed to set type of service option");
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos,
sizeof(tos)) < 0) {
Post by Naveen Singh
+ connman_error("Failed to set type of service
option");
Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char *server)
g_free(timeserver);
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
start_ntp(timeserver);
Patrik
Regards
Naveen
Cheers,
Jukka
Naveen Singh
2015-10-02 17:20:19 UTC
Permalink
Hi Jukka,

On Fri, Oct 2, 2015 at 12:29 AM, Jukka Rissanen <
Post by Jukka Rissanen
Hi Naveen,
On Thu, Oct 1, 2015 at 6:26 AM, Patrik Flykt <
Post by Naveen Singh
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that timeserver is
always an IPv4 address. If there is an IPv6 timeserver then the
socket
Post by Naveen Singh
Post by Naveen Singh
operation would fail with error as Permission denied (13). This
change in
Post by Naveen Singh
Post by Naveen Singh
ntp.c ensures that code works fine with both IPv4 and IPv6 address.
---
src/ntp.c | 129
++++++++++++++++++++++++++++++++++++++++++++++++--------------
Post by Naveen Singh
1 file changed, 101 insertions(+), 28 deletions(-)
This was patch version 2, so the next one is version 3. Add the version
to the Subject: line as [PATCH v3], this can be done with
'git ... --subject-prefix="PATCH v3" ...'.
Post by Naveen Singh
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
02110-1301 USA
Post by Naveen Singh
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server, uint32_t
timeout);
Post by Naveen Singh
Post by Naveen Singh
+static void send_packet(int fd, const char *server, uint32_t
timeout,
Post by Naveen Singh
int family);
Post by Naveen Singh
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean send_timeout(gpointer
user_data)
Post by Naveen Singh
Post by Naveen Singh
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver, timeout << 1);
+ send_packet(transmit_fd, timeserver, timeout << 1,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
-static void send_packet(int fd, const char *server, uint32_t
timeout)
Post by Naveen Singh
Post by Naveen Singh
+static void send_packet(int fd, const char *server, uint32_t
timeout,
Post by Naveen Singh
int family)
Instead of supplying a char *server, why don't we give a struct
sockaddr* instead? What's the purpose of carrying the server
"name" (nah, the IP address as a string), if it is already resolved in
the calling function? BTW, it is available as timeserver_addr, so do we
really need any of this information here?
Yes it makes sense but I am not sure sockaddr * is the one we should us.
It
will not be able to hold IPv6 address which is 16 bytes. So I guess
sockaddr_storage* would be the right thing. Do you agree? If you look i
changed type of timeserver_addr to sockaddr_storage. Do you agree with
this?
/* Structure describing a generic socket address. */
struct sockaddr
{
__SOCKADDR_COMMON (sa_); /* Common data: address family and
length.
*/
char sa_data[14]; /* Address data. */
};
You should never instantiate (== have a variable "struct sockaddr
myaddr") because there is not enough space as you noticed. You either
have a "struct sockaddr_in myaddr" or "struct sockaddr_in6 myaddr" and
then you just cast "struct sockaddr *" to correct variable.
There is no need to use sockaddr_storage here.
Exactly this is what I am saying.My concern is for the type of
timeserver_addr which is used for storing the IP address of the server. It
cannot be sockaddr. It needs to be sockaddr_storage. Where we know what IP
address we are using we can use sockaddr_in or sockaddr_in6 and hen cast it
to sockaddr *.

Does this make sense? Do you agree?
Post by Jukka Rissanen
Post by Naveen Singh
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
* At some point, we could specify the actual system precision
@@ -168,10 +172,29 @@ static void send_packet(int fd, const char
*server, uint32_t timeout)
Post by Naveen Singh
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
The above is unnecessary. You already have timeserver_addr set or the
function gets a sockaddr * to use.
Agreed. If we pass sockaddr_storage*.
No, sockaddr * is enough.
Post by Naveen Singh
Post by Naveen Singh
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd, const char
*server,
Post by Naveen Singh
uint32_t timeout)
Post by Naveen Singh
msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000);
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr, sizeof(addr));
+ addr, size);
Use the stored timeserver_addr or supplied sockaddr * family
information
Post by Naveen Singh
to set the size to either sizeof(struct sockaddr_in) or sizeof(struct
sockaddr_in6).
I guess this is what i am doing. But will double confirm.
Post by Naveen Singh
Post by Naveen Singh
if (len < 0) {
connman_error("Time request for server %s failed
(%d/%s)",
Post by Naveen Singh
Post by Naveen Singh
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer user_data)
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT,
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel
*channel,
Post by Naveen Singh
GIOCondition condition,
Post by Naveen Singh
gpointer
user_data)
Post by Naveen Singh
Post by Naveen Singh
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean received_data(GIOChannel
*channel,
Post by Naveen Singh
GIOCondition condition,
Post by Naveen Singh
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr !=
sender_addr.sin_addr.s_addr)
Post by Naveen Singh
Post by Naveen Singh
- /* only accept messages from the timeserver */
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr != ((struct sockaddr_in
*)&sender_addr)->sin_addr.s_addr)
Post by Naveen Singh
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ ((struct sockaddr_in6
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ 16) != 0)
Use memcmp for both of these. First check address families, if they
match the size can be deduced (for example as above) and have only a
single memcmp to do the matching.
Agreed.
Post by Naveen Singh
Post by Naveen Singh
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean received_data(GIOChannel
*channel,
Post by Naveen Singh
GIOCondition condition,
Post by Naveen Singh
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;
if (!server)
return;
- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
Before throwing away info, the address is already available as
->sin6_addr or ->sin_addr. See man getaddrinfo. Store the address and
the address family in timeserver_addr.
I guess this may not be correct as getaddrinfo gives us a pointer to
sockaddr which may not be able to contain the 16 byte IPv6 address. Do
you
agree? I guess using inet_pton is the best way.
Post by Naveen Singh
Post by Naveen Singh
+
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server socket");
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv4 address string");
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv6 address string");
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }
No need for the above, look into the 'info' supplied to getaddrinfo()
above.
Post by Naveen Singh
- if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr))
<
Post by Naveen Singh
0) {
Post by Naveen Singh
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) {
connman_error("Failed to bind time server socket");
close(transmit_fd);
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos,
sizeof(tos))
Post by Naveen Singh
< 0) {
Post by Naveen Singh
- connman_error("Failed to set type of service option");
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos,
sizeof(tos)) < 0) {
Post by Naveen Singh
+ connman_error("Failed to set type of service
option");
Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP,
&timestamp,
Post by Naveen Singh
Post by Naveen Singh
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
}
int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char *server)
g_free(timeserver);
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr = inet_addr(server);
start_ntp(timeserver);
Patrik
Regards
Naveen
Cheers,
Jukka
Jukka Rissanen
2015-10-05 07:49:15 UTC
Permalink
Hi Naveen,
Post by Naveen Singh
Hi Jukka,
On Fri, Oct 2, 2015 at 12:29 AM, Jukka Rissanen
Hi Naveen,
On Thu, Oct 1, 2015 at 6:26 AM, Patrik Flykt
Post by Patrik Flykt
Hi,
Post by Naveen Singh
Current NTP code is written with an assumption that
timeserver is
Post by Patrik Flykt
Post by Naveen Singh
always an IPv4 address. If there is an IPv6 timeserver
then the socket
Post by Patrik Flykt
Post by Naveen Singh
operation would fail with error as Permission denied
(13). This change in
Post by Patrik Flykt
Post by Naveen Singh
ntp.c ensures that code works fine with both IPv4 and
IPv6 address.
Post by Patrik Flykt
Post by Naveen Singh
---
src/ntp.c | 129
++++++++++++++++++++++++++++++++++++++++++++++++--------------
Post by Patrik Flykt
Post by Naveen Singh
1 file changed, 101 insertions(+), 28 deletions(-)
This was patch version 2, so the next one is version 3.
Add the version
Post by Patrik Flykt
to the Subject: line as [PATCH v3], this can be done with
'git ... --subject-prefix="PATCH v3" ...'.
Post by Naveen Singh
diff --git a/src/ntp.c b/src/ntp.c
index 2c313a4..a55365d 100644
--- a/src/ntp.c
+++ b/src/ntp.c
@@ -18,7 +18,6 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor,
Boston, MA
Post by Patrik Flykt
02110-1301 USA
Post by Naveen Singh
*
*/
-
#ifdef HAVE_CONFIG_H
#include <config.h>
#endif
@@ -34,6 +33,7 @@
#include <netinet/in.h>
#include <netinet/ip.h>
#include <arpa/inet.h>
+#include <netdb.h>
#include <glib.h>
@@ -117,12 +117,12 @@ static struct timespec mtx_time;
static int transmit_fd = 0;
static char *timeserver = NULL;
-static struct sockaddr_in timeserver_addr;
+static struct sockaddr_storage timeserver_addr;
static gint poll_id = 0;
static gint timeout_id = 0;
static guint retries = 0;
-static void send_packet(int fd, const char *server,
uint32_t timeout);
Post by Patrik Flykt
Post by Naveen Singh
+static void send_packet(int fd, const char *server,
uint32_t timeout,
Post by Patrik Flykt
int family);
Post by Naveen Singh
static void next_server(void)
{
@@ -143,17 +143,21 @@ static gboolean
send_timeout(gpointer user_data)
Post by Patrik Flykt
Post by Naveen Singh
if (retries++ == NTP_SEND_RETRIES)
next_server();
else
- send_packet(transmit_fd, timeserver,
timeout << 1);
Post by Patrik Flykt
Post by Naveen Singh
+ send_packet(transmit_fd, timeserver,
timeout << 1,
Post by Patrik Flykt
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
-static void send_packet(int fd, const char *server,
uint32_t timeout)
Post by Patrik Flykt
Post by Naveen Singh
+static void send_packet(int fd, const char *server,
uint32_t timeout,
Post by Patrik Flykt
int family)
Instead of supplying a char *server, why don't we give a
struct
Post by Patrik Flykt
sockaddr* instead? What's the purpose of carrying the
server
Post by Patrik Flykt
"name" (nah, the IP address as a string), if it is already
resolved in
Post by Patrik Flykt
the calling function? BTW, it is available as
timeserver_addr, so do we
Post by Patrik Flykt
really need any of this information here?
Yes it makes sense but I am not sure sockaddr * is the one
we should us. It
will not be able to hold IPv6 address which is 16 bytes. So
I guess
sockaddr_storage* would be the right thing. Do you agree? If
you look i
changed type of timeserver_addr to sockaddr_storage. Do you
agree with this?
/* Structure describing a generic socket address. */
struct sockaddr
{
__SOCKADDR_COMMON (sa_); /* Common data: address
family and length.
*/
char sa_data[14]; /* Address data. */
};
You should never instantiate (== have a variable "struct sockaddr
myaddr") because there is not enough space as you noticed. You either
have a "struct sockaddr_in myaddr" or "struct sockaddr_in6 myaddr" and
then you just cast "struct sockaddr *" to correct variable.
There is no need to use sockaddr_storage here.
Exactly this is what I am saying.My concern is for the type of
timeserver_addr which is used for storing the IP address of the
server. It cannot be sockaddr. It needs to be sockaddr_storage. Where
we know what IP address we are using we can use sockaddr_in or
sockaddr_in6 and hen cast it to sockaddr *.
Normally in socket programming you instantiate either sockaddr_in or
sockaddr_in6 variable, and then cast sockaddr * to that variable and
pass that pointer around. At least I would prefer that way instead of
creating sockaddr_storage variable for this purpose. So in practice
there is no need to create sockaddr_storage variable.
Post by Naveen Singh
Does this make sense? Do you agree?
Post by Patrik Flykt
Post by Naveen Singh
{
struct ntp_msg msg;
- struct sockaddr_in addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct sockaddr * addr;
struct timeval transmit_timeval;
ssize_t len;
+ int size;
+ unsigned char * addrptr;
/*
* At some point, we could specify the actual
system precision
Post by Patrik Flykt
Post by Naveen Singh
@@ -168,10 +172,29 @@ static void send_packet(int fd,
const char
Post by Patrik Flykt
*server, uint32_t timeout)
Post by Naveen Singh
msg.poll = 10; // max
msg.precision = NTP_PRECISION_S;
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
- addr.sin_port = htons(123);
- addr.sin_addr.s_addr = inet_addr(server);
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = AF_INET;
+ in4addr.sin_port = htons(123);
+ size = sizeof(in4addr);
+ addrptr = (unsigned char
*)&in4addr.sin_addr.s_addr;
Post by Patrik Flykt
Post by Naveen Singh
+ addr = (struct sockaddr *)&in4addr;
+ } else if (family == AF_INET6){
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = AF_INET6;
+ in6addr.sin6_port = htons(123);
+ size = sizeof(in6addr);
+ addrptr =
in6addr.sin6_addr.__in6_u.__u6_addr8;
Post by Patrik Flykt
Post by Naveen Singh
+ addr = (struct sockaddr *)&in6addr;
+ } else {
+ DBG("wrong family type");
+ return;
+ }
+ if (inet_pton(family, server, addrptr) == 0)
+ {
+ DBG("cannot convert ip address string");
+ return;
+ }
The above is unnecessary. You already have timeserver_addr
set or the
Post by Patrik Flykt
function gets a sockaddr * to use.
Agreed. If we pass sockaddr_storage*.
No, sockaddr * is enough.
Post by Patrik Flykt
Post by Naveen Singh
gettimeofday(&transmit_timeval, NULL);
clock_gettime(CLOCK_MONOTONIC, &mtx_time);
@@ -180,7 +203,7 @@ static void send_packet(int fd,
const char *server,
Post by Patrik Flykt
uint32_t timeout)
Post by Naveen Singh
msg.xmttime.fraction =
htonl(transmit_timeval.tv_usec * 1000);
Post by Patrik Flykt
Post by Naveen Singh
len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT,
- &addr,
sizeof(addr));
Post by Patrik Flykt
Post by Naveen Singh
+ addr,
size);
Post by Patrik Flykt
Use the stored timeserver_addr or supplied sockaddr *
family information
Post by Patrik Flykt
to set the size to either sizeof(struct sockaddr_in) or
sizeof(struct
Post by Patrik Flykt
sockaddr_in6).
I guess this is what i am doing. But will double confirm.
Post by Patrik Flykt
Post by Naveen Singh
if (len < 0) {
connman_error("Time request for server %s
failed (%d/%s)",
Post by Patrik Flykt
Post by Naveen Singh
server, errno, strerror(errno));
@@ -213,7 +236,7 @@ static gboolean next_poll(gpointer
user_data)
Post by Patrik Flykt
Post by Naveen Singh
if (!timeserver || transmit_fd == 0)
return FALSE;
- send_packet(transmit_fd, timeserver,
NTP_SEND_TIMEOUT);
Post by Patrik Flykt
Post by Naveen Singh
+ send_packet(transmit_fd, timeserver,
NTP_SEND_TIMEOUT,
Post by Patrik Flykt
timeserver_addr.ss_family);
Post by Naveen Singh
return FALSE;
}
@@ -363,7 +386,7 @@ static gboolean
received_data(GIOChannel *channel,
Post by Patrik Flykt
GIOCondition condition,
gpointer user_data)
Post by Patrik Flykt
Post by Naveen Singh
{
unsigned char buf[128];
- struct sockaddr_in sender_addr;
+ struct sockaddr_storage sender_addr;
struct msghdr msg;
struct iovec iov;
struct cmsghdr *cmsg;
@@ -396,9 +419,18 @@ static gboolean
received_data(GIOChannel *channel,
Post by Patrik Flykt
GIOCondition condition,
Post by Naveen Singh
if (len < 0)
return TRUE;
- if (timeserver_addr.sin_addr.s_addr !=
sender_addr.sin_addr.s_addr)
Post by Patrik Flykt
Post by Naveen Singh
- /* only accept messages from the
timeserver */
Post by Patrik Flykt
Post by Naveen Singh
+ if (sender_addr.ss_family == AF_INET) {
+ if (((struct sockaddr_in
*)&timeserver_addr)->sin_addr.s_addr != ((struct
sockaddr_in
Post by Patrik Flykt
*)&sender_addr)->sin_addr.s_addr)
Post by Naveen Singh
+ return TRUE;
+ } else if(sender_addr.ss_family == AF_INET6) {
+ if (memcmp(((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ ((struct
sockaddr_in6
Post by Patrik Flykt
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
Post by Naveen Singh
+ 16) != 0)
Use memcmp for both of these. First check address
families, if they
Post by Patrik Flykt
match the size can be deduced (for example as above) and
have only a
Post by Patrik Flykt
single memcmp to do the matching.
Agreed.
Post by Patrik Flykt
Post by Naveen Singh
+ return TRUE;
+ } else {
+ connman_error("Not a valid family type");
return TRUE;
+ }
tv = NULL;
clock_gettime(CLOCK_MONOTONIC, &mrx_time);
@@ -422,36 +454,78 @@ static gboolean
received_data(GIOChannel *channel,
Post by Patrik Flykt
GIOCondition condition,
Post by Naveen Singh
static void start_ntp(char *server)
{
GIOChannel *channel;
- struct sockaddr_in addr;
+ struct sockaddr * addr;
+ struct sockaddr_in in4addr;
+ struct sockaddr_in6 in6addr;
+ struct addrinfo hint;
+ struct addrinfo *info;
+ int family;
int tos = IPTOS_LOWDELAY, timestamp = 1;
+ int size;
+ int ret;
if (!server)
return;
- DBG("server %s", server);
+ memset(&hint, 0, sizeof(hint));
+ hint.ai_family = AF_UNSPEC;
+ hint.ai_flags = AI_NUMERICHOST;
+ ret = getaddrinfo(server, NULL, &hint, &info);
+
+ if (ret) {
+ connman_error("cannot get server info");
+ return;
+ }
+
+ family = info->ai_family;
+ freeaddrinfo(info);
Before throwing away info, the address is already
available as
Post by Patrik Flykt
->sin6_addr or ->sin_addr. See man getaddrinfo. Store the
address and
Post by Patrik Flykt
the address family in timeserver_addr.
I guess this may not be correct as getaddrinfo gives us a
pointer to
sockaddr which may not be able to contain the 16 byte IPv6
address. Do you
agree? I guess using inet_pton is the best way.
Post by Patrik Flykt
Post by Naveen Singh
+
+ DBG("server %s family %d", server, family);
if (channel_watch > 0)
goto send;
- transmit_fd = socket(PF_INET, SOCK_DGRAM |
SOCK_CLOEXEC, 0);
Post by Patrik Flykt
Post by Naveen Singh
- if (transmit_fd < 0) {
+ transmit_fd = socket(family, SOCK_DGRAM |
SOCK_CLOEXEC, 0);
Post by Patrik Flykt
Post by Naveen Singh
+ if (transmit_fd <= 0) {
connman_error("Failed to open time server
socket");
Post by Patrik Flykt
Post by Naveen Singh
return;
}
- memset(&addr, 0, sizeof(addr));
- addr.sin_family = AF_INET;
+ if (family == AF_INET) {
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ if (inet_pton(family, server, &(((struct
sockaddr_in
Post by Patrik Flykt
*)&timeserver_addr)->sin_addr.s_addr)) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv4 address
string");
Post by Patrik Flykt
Post by Naveen Singh
+ }
+ } else if (family == AF_INET6) {
+ memset(&in6addr, 0, sizeof(in6addr));
+ in6addr.sin6_family = family;
+ addr = (struct sockaddr *)&in6addr;
+ size = sizeof(in6addr);
+ if (inet_pton(family, server, ((struct
sockaddr_in6
Post by Patrik Flykt
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
Post by Naveen Singh
+ DBG("cannot convert ipv6 address
string");
Post by Patrik Flykt
Post by Naveen Singh
+ }
+ } else {
+ connman_error("Family not correct");
+ return;
+ }
No need for the above, look into the 'info' supplied to
getaddrinfo()
Post by Patrik Flykt
above.
Post by Naveen Singh
- if (bind(transmit_fd, (struct sockaddr *) &addr,
sizeof(addr)) <
Post by Patrik Flykt
0) {
Post by Naveen Singh
+ timeserver_addr.ss_family = family;
+
+ if (bind(transmit_fd, (struct sockaddr *) addr,
size) < 0) {
Post by Patrik Flykt
Post by Naveen Singh
connman_error("Failed to bind time server
socket");
Post by Patrik Flykt
Post by Naveen Singh
close(transmit_fd);
return;
}
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS,
&tos, sizeof(tos))
Post by Patrik Flykt
< 0) {
Post by Naveen Singh
- connman_error("Failed to set type of
service option");
Post by Patrik Flykt
Post by Naveen Singh
- close(transmit_fd);
- return;
+ if (family == AF_INET) {
+ if (setsockopt(transmit_fd, IPPROTO_IP,
IP_TOS, &tos,
Post by Patrik Flykt
sizeof(tos)) < 0) {
Post by Naveen Singh
+ connman_error("Failed to set type
of service
Post by Patrik Flykt
option");
Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET,
SO_TIMESTAMP, &timestamp,
Post by Patrik Flykt
Post by Naveen Singh
@@ -479,7 +553,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server,
NTP_SEND_TIMEOUT);
Post by Patrik Flykt
Post by Naveen Singh
+ send_packet(transmit_fd, server, NTP_SEND_TIMEOUT,
family);
Post by Patrik Flykt
Post by Naveen Singh
}
int __connman_ntp_start(char *server)
@@ -493,7 +567,6 @@ int __connman_ntp_start(char
*server)
Post by Patrik Flykt
Post by Naveen Singh
g_free(timeserver);
timeserver = g_strdup(server);
- timeserver_addr.sin_addr.s_addr =
inet_addr(server);
Post by Patrik Flykt
Post by Naveen Singh
start_ntp(timeserver);
Patrik
Regards
Naveen
Cheers,
Jukka
Patrik Flykt
2015-10-05 08:05:13 UTC
Permalink
Post by Jukka Rissanen
Post by Naveen Singh
Exactly this is what I am saying.My concern is for the type of
timeserver_addr which is used for storing the IP address of the
server. It cannot be sockaddr. It needs to be sockaddr_storage.
Where
Post by Naveen Singh
we know what IP address we are using we can use sockaddr_in or
sockaddr_in6 and hen cast it to sockaddr *.
Normally in socket programming you instantiate either sockaddr_in or
sockaddr_in6 variable, and then cast sockaddr * to that variable and
pass that pointer around. At least I would prefer that way instead of
creating sockaddr_storage variable for this purpose. So in practice
there is no need to create sockaddr_storage variable.
And as we know that a sockaddr_in6 has the space to hold also
sockaddr_in, sockaddr_in6 is a decently sized data structure to hold all
addresses. Provide this sockaddr_in6 as a sockaddr * to the functions.
They can cast it back to the needed type.

Cheers,

Patrik

Loading...