Hi Jukka,
On Fri, Oct 2, 2015 at 12:29 AM, Jukka Rissanen <
jukka.rissa...@linux.intel.com> wrote:
> Hi Naveen,
>
> On to, 2015-10-01 at 23:00 -0700, Naveen Singh wrote:
> > On Thu, Oct 1, 2015 at 6:26 AM, Patrik Flykt <
> patrik.fl...@linux.intel.com>
> > wrote:
> >
> > >
> > > Hi,
> > >
> > > On Wed, 2015-09-30 at 22:08 -0700, Naveen Singh wrote:
> > > > From: Naveen Singh <nav...@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(-)
> > >
> > > 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" ...'.
> > >
> > > > 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?
> > >
> >
> > 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?
>
> >
> > > > {
> > > > 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;
> > > > + }
> > >
> > > 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.
>
> >
> > >
> > > > 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).
> > >
> > I guess this is what i am doing. But will double confirm.
> >
> > >
> > > > 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
> > > *)×erver_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
> > > *)×erver_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.
> > >
> > Agreed.
> >
> > >
> > > > + 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.
> > >
> > > 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.
> >
> > > > +
> > > > + 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
> > > *)×erver_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
> > > *)×erver_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.
> > >
> > > > - 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,
> ×tamp,
> > > > @@ -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
> > >
> > > Regards
> > >
> > Naveen
> >
>
> Cheers,
> Jukka
>
>
>
_______________________________________________
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
