[PATCH wpan-tools 0/3] wpan-ping address handling rework and server simplification

[PATCH wpan-tools 0/3] wpan-ping address handling rework and server simplification

[Stefan Schmidt]

Hello.

This needs to go on top of the extended address handling patchset.

Some unification of the address handling and as nice feature a simplified server
side where no destination needs to be given as parameter.

If you have remarks I should be able to reply next week as I will be off for a long
weekend in a few hours.

Stefan Schmidt (3):
  wpan-ping: Reduce packet recv timeout to 500ms from 2s
  wpan-ping: unify src and dst address handling
  wpan-ping: use recvfrom and sendto isntead of connecting the socket

 wpan-ping/wpan-ping.c | 137 ++++++++++++++++++++++++--------------------------
 1 file changed, 65 insertions(+), 72 deletions(-)

regards
Stefan Schmidt

-- 
2.4.3

[PATCH wpan-tools 1/3] wpan-ping: Reduce packet recv timeout to 500ms from 2s

[Stefan Schmidt]

Under normal condistions a full length 802154 frame takes areound 18ms rtt.
Setting the timeout to 2s is way to high so we reduce it to 500ms.

Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 wpan-ping/wpan-ping.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/wpan-ping/wpan-ping.c b/wpan-ping/wpan-ping.c
index c83a0ad..d3f44f1 100644
--- a/wpan-ping/wpan-ping.c
+++ b/wpan-ping/wpan-ping.c
@@ -248,9 +248,9 @@ static int measure_roundtrip(struct config *conf, int sd) {
 			conf->dst_addr, conf->pan_id, conf->packet_len);
 	buf = (unsigned char *)malloc(MAX_PAYLOAD_LEN);
 
-	/* 2 seconds packet receive timeout */
-	timeout.tv_sec = 2;
-	timeout.tv_usec = 0;
+	/* 500ms seconds packet receive timeout */
+	timeout.tv_sec = 0;
+	timeout.tv_usec = 500000;
 	setsockopt(sd, SOL_SOCKET, SO_RCVTIMEO, (struct timeval *)&timeout,sizeof(struct timeval));
 
 	count = 0;
@@ -295,7 +295,7 @@ static int measure_roundtrip(struct config *conf, int sd) {
 				fprintf(stdout, "%i bytes from 0x%04x seq=%i time=%.1f ms\n", ret,
 					conf->dst_addr, (int)seq_num, (float)usec/1000);
 		} else
-			fprintf(stderr, "Hit 2s packet timeout\n");
+			fprintf(stderr, "Hit 500 ms packet timeout\n");
 	}
 
 	if (count)
-- 
2.4.3

[PATCH wpan-tools 2/3] wpan-ping: unify src and dst address handling

[Stefan Schmidt]

Instead of having parts of the sockaddr struct parsed and kept in different
places we put it all in the correct struct from the start now. All local
information for the src address are requested over netlink while the dst
address is parsed from the commandline. As the PAN ID has to be the same we
take this also from netlink for dst.

Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 wpan-ping/wpan-ping.c | 86 +++++++++++++++++++++++----------------------------
 1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/wpan-ping/wpan-ping.c b/wpan-ping/wpan-ping.c
index d3f44f1..5432bfa 100644
--- a/wpan-ping/wpan-ping.c
+++ b/wpan-ping/wpan-ping.c
@@ -86,16 +86,13 @@ static const struct option perf_long_opts[] = {
 struct config {
 	char packet_len;
 	unsigned short packets;
-	uint16_t dst_addr;
-	uint16_t short_addr;
-	uint16_t pan_id;
-	uint8_t dst_extended[IEEE802154_ADDR_LEN];
-	uint8_t src_extended[IEEE802154_ADDR_LEN];
 	bool extended;
 	bool server;
 	char *interface;
 	struct nl_sock *nl_sock;
 	int nl802154_id;
+	struct sockaddr_ieee802154 src;
+	struct sockaddr_ieee802154 dst;
 };
 
 extern char *optarg;
@@ -168,10 +165,17 @@ static int nl_msg_cb(struct nl_msg* msg, void* arg)
 	    || !attrs[NL802154_ATTR_EXTENDED_ADDR])
 		return NL_SKIP;
 
-	conf->short_addr = nla_get_u16(attrs[NL802154_ATTR_SHORT_ADDR]);
-	conf->pan_id = nla_get_u16(attrs[NL802154_ATTR_PAN_ID]);
-	temp = htobe64(nla_get_u64(attrs[NL802154_ATTR_EXTENDED_ADDR]));
-	memcpy(&conf->src_extended, &temp, IEEE802154_ADDR_LEN);
+	conf->src.family = AF_IEEE802154;
+	conf->src.addr.pan_id = conf->dst.addr.pan_id = nla_get_u16(attrs[NL802154_ATTR_PAN_ID]);
+
+	if (!conf->extended) {
+		conf->src.addr.addr_type = IEEE802154_ADDR_SHORT;
+		conf->src.addr.short_addr = nla_get_u16(attrs[NL802154_ATTR_SHORT_ADDR]);
+	} else {
+		conf->src.addr.addr_type = IEEE802154_ADDR_LONG;
+		temp = htobe64(nla_get_u64(attrs[NL802154_ATTR_EXTENDED_ADDR]));
+		memcpy(&conf->src.addr.hwaddr, &temp, IEEE802154_ADDR_LEN);
+	}
 
 	return NL_SKIP;
 }
@@ -238,14 +242,14 @@ static int measure_roundtrip(struct config *conf, int sd) {
 	char addr[24];
 
 	if (conf->extended)
-		print_address(addr, conf->dst_extended);
+		print_address(addr, conf->dst.addr.hwaddr);
 
 	if (conf->extended)
 		fprintf(stdout, "PING %s (PAN ID 0x%04x) %i data bytes\n",
-			addr, conf->pan_id, conf->packet_len);
+			addr, conf->dst.addr.pan_id, conf->packet_len);
 	else
 		fprintf(stdout, "PING 0x%04x (PAN ID 0x%04x) %i data bytes\n",
-			conf->dst_addr, conf->pan_id, conf->packet_len);
+			conf->dst.addr.short_addr, conf->dst.addr.pan_id, conf->packet_len);
 	buf = (unsigned char *)malloc(MAX_PAYLOAD_LEN);
 
 	/* 500ms seconds packet receive timeout */
@@ -293,7 +297,7 @@ static int measure_roundtrip(struct config *conf, int sd) {
 					addr, (int)seq_num, (float)usec/1000);
 			else
 				fprintf(stdout, "%i bytes from 0x%04x seq=%i time=%.1f ms\n", ret,
-					conf->dst_addr, (int)seq_num, (float)usec/1000);
+					conf->dst.addr.short_addr, (int)seq_num, (float)usec/1000);
 		} else
 			fprintf(stderr, "Hit 500 ms packet timeout\n");
 	}
@@ -311,7 +315,7 @@ static int measure_roundtrip(struct config *conf, int sd) {
 	if (conf->extended)
 		fprintf(stdout, "\n--- %s ping statistics ---\n", addr);
 	else
-		fprintf(stdout, "\n--- 0x%04x ping statistics ---\n", conf->dst_addr);
+		fprintf(stdout, "\n--- 0x%04x ping statistics ---\n", conf->dst.addr.short_addr);
 	fprintf(stdout, "%i packets transmitted, %i received, %.0f%% packet loss\n",
 		conf->packets, count, packet_loss);
 	fprintf(stdout, "rtt min/avg/max = %.3f/%.3f/%.3f ms\n", rtt_min, rtt_avg, rtt_max);
@@ -345,7 +349,6 @@ static void init_server(struct config *conf, int sd) {
 static int init_network(struct config *conf) {
 	int sd;
 	int ret;
-	struct sockaddr_ieee802154 a;
 
 	sd = socket(PF_IEEE802154, SOCK_DGRAM, 0);
 	if (sd < 0) {
@@ -353,35 +356,15 @@ static int init_network(struct config *conf) {
 		return 1;
 	}
 
-	get_interface_info(conf);
-
-	a.family = AF_IEEE802154;
-	a.addr.pan_id = conf->pan_id;
-	if (conf->extended)
-		a.addr.addr_type = IEEE802154_ADDR_LONG;
-	else
-		a.addr.addr_type = IEEE802154_ADDR_SHORT;
-
-
 	/* Bind socket on this side */
-	if (conf->extended)
-		memcpy(&a.addr.hwaddr, &conf->src_extended, IEEE802154_ADDR_LEN);
-	else
-		a.addr.short_addr = conf->short_addr;
-
-	ret = bind(sd, (struct sockaddr *)&a, sizeof(a));
+	ret = bind(sd, (struct sockaddr *)&conf->src, sizeof(conf->src));
 	if (ret) {
 		perror("bind");
 		return 1;
 	}
 
 	/* Connect to other side */
-	if (conf->extended)
-		memcpy(&a.addr.hwaddr, &conf->dst_extended, IEEE802154_ADDR_LEN);
-	else
-		a.addr.short_addr = conf->dst_addr;
-
-	ret = connect(sd, (struct sockaddr *)&a, sizeof(a));
+	ret = connect(sd, (struct sockaddr *)&conf->dst, sizeof(conf->dst));
 	if (ret) {
 		perror("connect");
 		return 1;
@@ -389,23 +372,29 @@ static int init_network(struct config *conf) {
 
 	if (conf->server)
 		init_server(conf, sd);
-
-	measure_roundtrip(conf, sd);
+	else
+		measure_roundtrip(conf, sd);
 
 	shutdown(sd, SHUT_RDWR);
 	close(sd);
 	return 0;
 }
 
-static int parse_address(struct config *conf, char *arg)
+static int parse_dst_addr(struct config *conf, char *arg)
 {
 	int i;
 
+	/* PAN ID is filled from netlink in get_interface_info */
+	conf->dst.family = AF_IEEE802154;
+
 	if (!conf->extended) {
-		conf->dst_addr = strtol(arg, NULL, 16);
+		conf->dst.addr.addr_type = IEEE802154_ADDR_SHORT;
+		conf->dst.addr.short_addr = strtol(arg, NULL, 16);
 		return 0;
 	}
 
+	conf->dst.addr.addr_type = IEEE802154_ADDR_LONG;
+
 	for (i = 0; i < IEEE802154_ADDR_LEN; i++) {
 		int temp;
 		char *cp = strchr(arg, ':');
@@ -418,7 +407,7 @@ static int parse_address(struct config *conf, char *arg)
 		if (temp < 0 || temp > 255)
 			return -1;
 
-		conf->dst_extended[i] = temp;
+		conf->dst.addr.hwaddr[i] = temp;
 		if (!cp)
 			break;
 		arg = cp;
@@ -432,9 +421,9 @@ static int parse_address(struct config *conf, char *arg)
 int main(int argc, char *argv[]) {
 	int c;
 	struct config *conf;
-	char *address;
+	char *dst_addr;
 
-	conf = (struct config *) malloc(sizeof(struct config));
+	conf = malloc(sizeof(struct config));
 
 	/* Default to interface wpan0 if nothing else is given */
 	conf->interface = "wpan0";
@@ -461,14 +450,14 @@ int main(int argc, char *argv[]) {
 			break;
 		switch(c) {
 		case 'a':
-			address = optarg;
+			dst_addr = optarg;
 			break;
 		case 'e':
 			conf->extended = true;
 			break;
 		case 'd':
+			dst_addr = optarg;
 			conf->server = true;
-			address = optarg;
 			break;
 		case 'c':
 			conf->packets = atoi(optarg);
@@ -496,11 +485,12 @@ int main(int argc, char *argv[]) {
 		}
 	}
 
-	if (parse_address(conf, address) < 0) {
+	get_interface_info(conf);
+
+	if (parse_dst_addr(conf, dst_addr) < 0) {
 		fprintf(stderr, "Address given in wrong format.\n");
 		return 1;
 	}
-
 	init_network(conf);
 	free(conf);
 	return 0;
-- 
2.4.3

Re: [PATCH wpan-tools 2/3] wpan-ping: unify src and dst address handling

[Alexander Aring]

On Fri, Jun 19, 2015 at 12:27:38AM +0200, Stefan Schmidt wrote:
> Instead of having parts of the sockaddr struct parsed and kept in different
> places we put it all in the correct struct from the start now. All local
> information for the src address are requested over netlink while the dst
> address is parsed from the commandline. As the PAN ID has to be the same we
> take this also from netlink for dst.
> 
> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
> ---
>  wpan-ping/wpan-ping.c | 86 +++++++++++++++++++++++----------------------------
>  1 file changed, 38 insertions(+), 48 deletions(-)
> 
> diff --git a/wpan-ping/wpan-ping.c b/wpan-ping/wpan-ping.c
> index d3f44f1..5432bfa 100644
> --- a/wpan-ping/wpan-ping.c
> +++ b/wpan-ping/wpan-ping.c
> @@ -86,16 +86,13 @@ static const struct option perf_long_opts[] = {
>  struct config {
>  	char packet_len;
>  	unsigned short packets;
> -	uint16_t dst_addr;
> -	uint16_t short_addr;
> -	uint16_t pan_id;
> -	uint8_t dst_extended[IEEE802154_ADDR_LEN];
> -	uint8_t src_extended[IEEE802154_ADDR_LEN];
>  	bool extended;
>  	bool server;
>  	char *interface;
>  	struct nl_sock *nl_sock;
>  	int nl802154_id;
> +	struct sockaddr_ieee802154 src;
> +	struct sockaddr_ieee802154 dst;
>  };
>  
>  extern char *optarg;
> @@ -168,10 +165,17 @@ static int nl_msg_cb(struct nl_msg* msg, void* arg)
>  	    || !attrs[NL802154_ATTR_EXTENDED_ADDR])
>  		return NL_SKIP;
>  
> -	conf->short_addr = nla_get_u16(attrs[NL802154_ATTR_SHORT_ADDR]);
> -	conf->pan_id = nla_get_u16(attrs[NL802154_ATTR_PAN_ID]);
> -	temp = htobe64(nla_get_u64(attrs[NL802154_ATTR_EXTENDED_ADDR]));
> -	memcpy(&conf->src_extended, &temp, IEEE802154_ADDR_LEN);
> +	conf->src.family = AF_IEEE802154;
> +	conf->src.addr.pan_id = conf->dst.addr.pan_id = nla_get_u16(attrs[NL802154_ATTR_PAN_ID]);
> +
> +	if (!conf->extended) {
> +		conf->src.addr.addr_type = IEEE802154_ADDR_SHORT;
> +		conf->src.addr.short_addr = nla_get_u16(attrs[NL802154_ATTR_SHORT_ADDR]);
> +	} else {
> +		conf->src.addr.addr_type = IEEE802154_ADDR_LONG;
> +		temp = htobe64(nla_get_u64(attrs[NL802154_ATTR_EXTENDED_ADDR]));
> +		memcpy(&conf->src.addr.hwaddr, &temp, IEEE802154_ADDR_LEN);
> +	}

I think what you do here is to get the src address, because _currently_
the DGRAM sockets of af_802154 need to specify the source address for
creating socket. This is some behaviour which Lennert already mentioned,
that this behaviour has no architecture. The kernel already know the
source address and can put it into the payload for DGRAM sockets.

Nevertheless what I really want to say is the bytordering. Please
remeber that the nl802154 for addresses is in little endian (like the
mac byte order). The DGRAM sockets are in host byte order.

I also already mentioned that this is really confusing and we should
decide later (maybe for next nl802154 upgrade cycle) to decide that we
should have only one behaviour.

Setting the mac address over ip:

"ip link set dev interface address XX:XX:XX:XX:XX:XX"

will require big endian (I suppose a2n ist address to network byte order
which big endian). See [0].


So now it's really confusing, because over "generic ip interface" you
will get big endian. Over nl802154 it's little endian and DGRAM is host
byteorder.

There are maybe reasons for let the nl802154 for MAC settings (which is
the address) in little endian, because the ip stuff do the same for the
network byteorder. It's hard to decide what we should do now, but at the
moment I agree it's confusing and we should open a new thread to talking
about.

The most users have little endian machines, so changing the nl802154
stuff from le to host-byteorder should not make big different, except
if we have big endian users. I know ARM has the possibility to run big
endian, but I saw never somebody which really runs linux on big endian
ARM.

- Alex

[0] http://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/ip/iplink.c#n426

[PATCH wpan-tools 3/3] wpan-ping: use recvfrom and sendto isntead of connecting the socket

[Stefan Schmidt]

While connecting makes sense when the dst address is known we make our
life easier if we pick the origin address from recvfrom on the server
side. This will eliminate the cumbersome dst parameter on the server
side. A simple ./wpan-ping -d will now respond to different clients.

Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 wpan-ping/wpan-ping.c | 49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/wpan-ping/wpan-ping.c b/wpan-ping/wpan-ping.c
index 5432bfa..6607474 100644
--- a/wpan-ping/wpan-ping.c
+++ b/wpan-ping/wpan-ping.c
@@ -71,7 +71,7 @@ struct sockaddr_ieee802154 {
 
 #ifdef HAVE_GETOPT_LONG
 static const struct option perf_long_opts[] = {
-	{ "daemon", required_argument, NULL, 'd' },
+	{ "daemon", no_argument, NULL, 'd' },
 	{ "address", required_argument, NULL, 'a' },
 	{ "extended", no_argument, NULL, 'e' },
 	{ "count", required_argument, NULL, 'c' },
@@ -100,7 +100,7 @@ extern char *optarg;
 void usage(const char *name) {
 	printf("Usage: %s OPTIONS\n"
 	"OPTIONS:\n"
-	"--daemon |-d client address\n"
+	"--daemon |-d\n"
 	"--address | -a server address\n"
 	"--extended | -e use extended addressing scheme 00:11:22:...\n"
 	"--count | -c number of packets\n"
@@ -261,7 +261,10 @@ static int measure_roundtrip(struct config *conf, int sd) {
 	for (i = 0; i < conf->packets; i++) {
 		generate_packet(buf, conf, i);
 		seq_num = (buf[2] << 8)| buf[3];
-		send(sd, buf, conf->packet_len, 0);
+		ret = sendto(sd, buf, conf->packet_len, 0, (struct sockaddr *)&conf->dst, sizeof(conf->dst));
+		if (ret < 0) {
+			perror("sendto");
+		}
 		gettimeofday(&start_time, NULL);
 		ret = recv(sd, buf, conf->packet_len, 0);
 		if (seq_num != ((buf[2] << 8)| buf[3])) {
@@ -327,21 +330,26 @@ static int measure_roundtrip(struct config *conf, int sd) {
 static void init_server(struct config *conf, int sd) {
 	ssize_t len;
 	unsigned char *buf;
-//	struct sockaddr_ieee802154 src;
-//	socklen_t addrlen;
+	struct sockaddr_ieee802154 src;
+	socklen_t addrlen;
 
-//	addrlen = sizeof(src);
+	addrlen = sizeof(src);
 
 	len = 0;
 	fprintf(stdout, "Server mode. Waiting for packets...\n");
 	buf = (unsigned char *)malloc(MAX_PAYLOAD_LEN);
 
 	while (1) {
-		//len = recvfrom(sd, buf, MAX_PAYLOAD_LEN, 0, (struct sockaddr *)&src, &addrlen);
-		len = recv(sd, buf, MAX_PAYLOAD_LEN, 0);
+		len = recvfrom(sd, buf, MAX_PAYLOAD_LEN, 0, (struct sockaddr *)&src, &addrlen);
+		if (len < 0) {
+			perror("recvfrom");
+		}
 		//dump_packet(buf, len);
 		/* Send same packet back */
-		send(sd, buf, len, 0);
+		len = sendto(sd, buf, len, 0, (struct sockaddr *)&src, addrlen);
+		if (len < 0) {
+			perror("sendto");
+		}
 	}
 	free(buf);
 }
@@ -363,13 +371,6 @@ static int init_network(struct config *conf) {
 		return 1;
 	}
 
-	/* Connect to other side */
-	ret = connect(sd, (struct sockaddr *)&conf->dst, sizeof(conf->dst));
-	if (ret) {
-		perror("connect");
-		return 1;
-	}
-
 	if (conf->server)
 		init_server(conf, sd);
 	else
@@ -419,7 +420,7 @@ static int parse_dst_addr(struct config *conf, char *arg)
 }
 
 int main(int argc, char *argv[]) {
-	int c;
+	int c, ret;
 	struct config *conf;
 	char *dst_addr;
 
@@ -442,9 +443,9 @@ int main(int argc, char *argv[]) {
 	while (1) {
 #ifdef HAVE_GETOPT_LONG
 		int opt_idx = -1;
-		c = getopt_long(argc, argv, "b:a:ec:s:i:d:vh", perf_long_opts, &opt_idx);
+		c = getopt_long(argc, argv, "b:a:ec:s:i:dvh", perf_long_opts, &opt_idx);
 #else
-		c = getopt(argc, argv, "b:a:ec:s:i:d:vh");
+		c = getopt(argc, argv, "b:a:ec:s:i:dvh");
 #endif
 		if (c == -1)
 			break;
@@ -456,7 +457,6 @@ int main(int argc, char *argv[]) {
 			conf->extended = true;
 			break;
 		case 'd':
-			dst_addr = optarg;
 			conf->server = true;
 			break;
 		case 'c':
@@ -487,9 +487,12 @@ int main(int argc, char *argv[]) {
 
 	get_interface_info(conf);
 
-	if (parse_dst_addr(conf, dst_addr) < 0) {
-		fprintf(stderr, "Address given in wrong format.\n");
-		return 1;
+	if (!conf->server) {
+		ret = parse_dst_addr(conf, dst_addr);
+		if (ret< 0) {
+			fprintf(stderr, "Address given in wrong format.\n");
+			return 1;
+		}
 	}
 	init_network(conf);
 	free(conf);
-- 
2.4.3

Re: [PATCH wpan-tools 0/3] wpan-ping address handling rework and server simplification

[Alexander Aring]

On Fri, Jun 19, 2015 at 12:27:36AM +0200, Stefan Schmidt wrote:
> Hello.
> 
> This needs to go on top of the extended address handling patchset.
> 
> Some unification of the address handling and as nice feature a simplified server
> side where no destination needs to be given as parameter.
> 
> If you have remarks I should be able to reply next week as I will be off for a long
> weekend in a few hours.
> 

It's okay, I am happy that we can have this nice tool in wpan-tools and
it's your playground. If it's compile, I am fine with that code.

Applied all, thanks.

- Alex