[PATCH] IPv6 Timeserver for NTP

[Naveen Singh]

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;
}

[Patrik Flykt]

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]

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]

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]

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]

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]

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]

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);

[Patrik Flykt]

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]

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]

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]

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]

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]

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]

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