Re: [RFC PATCH wpan-tools 2/2] wpan-hwsim: initial commit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

The patch subject should be better. Initial commit is something ok for a
complete new repo, but for a new tool a one line summary what it does
would be better.

Something like:
wpan-hwsim: hardware simulator configuration utility


I had you coded test run through our TravisCI and Coverity setup. It
reported three problems. See below.

A first review of the code can also be below inline with the code..


New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)


** CID 185498:  Control flow issues  (DEADCODE)
/wpan-hwsim/wpan-hwsim.c: 97 in hwsim_create()


________________________________________________________________________________________________________
*** CID 185498:  Control flow issues  (DEADCODE)
/wpan-hwsim/wpan-hwsim.c: 97 in hwsim_create()
91     	cb = nl_cb_alloc(NL_CB_DEFAULT);
92     	if (!cb)
93     		return -ENOMEM;
94
95     	msg = nlmsg_alloc();
96     	if (!cb) {
>>>     CID 185498:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "nl_cb_put(cb);".
97     		nl_cb_put(cb);
98     		return -ENOMEM;
99     	}
100
101     	genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, nlhwsim_id, 0, 0,
102     		    MAC802154_HWSIM_CMD_NEW_RADIO, 0);

** CID 185497:  Uninitialized variables  (UNINIT)


________________________________________________________________________________________________________
*** CID 185497:  Uninitialized variables  (UNINIT)
/wpan-hwsim/wpan-hwsim.c: 497 in main()
491     			idx2 = strtoul(argv[optind + 3], NULL, 0);
492     			if (idx2 == ULONG_MAX) {
493     				fprintf(stderr, "Invalid second radio index for edge
command\n");
494     				return EXIT_FAILURE;
495     			}
496
>>>     CID 185497:  Uninitialized variables  (UNINIT)
>>>     Using uninitialized value "lqi" when calling "hwsim_do_cmd".
497     			rc = hwsim_do_cmd(cmd, idx, idx2, lqi, 0, 0);
498     			if (rc < 0)
499     				return EXIT_FAILURE;
500     		}
501     	} else {
502     		fprintf(stderr, "Unknown command\n");

** CID 185496:  Control flow issues  (NO_EFFECT)
/wpan-hwsim/wpan-hwsim.c: 479 in main()


________________________________________________________________________________________________________
*** CID 185496:  Control flow issues  (NO_EFFECT)
/wpan-hwsim/wpan-hwsim.c: 479 in main()
473     				if (optind + 5 > argc) {
474     					fprintf(stderr, "LQI information missing\n");
475     					return EXIT_FAILURE;
476     				}
477
478     				lqi = strtoul(argv[optind + 4], NULL, 0);
>>>     CID 185496:  Control flow issues  (NO_EFFECT)
>>>     This less-than-zero comparison of an unsigned value is never
true. "lqi < 0UL".
479     				if (lqi == ULONG_MAX || lqi > 0xff || lqi < 0) {
480     					fprintf(stderr, "Invalid lqi value\n");
481     					return EXIT_FAILURE;
482     				}
483     			}
484


On 27.04.2018 23:24, Alexander Aring wrote:
> This patch adds initial support for wpan-hwsim utility to control the
> mac802154_hwsim driver over netlink.
> 
> Signed-off-by: Alexander Aring <aring@xxxxxxxxxxxx>
> ---
>  Makefile.am                  |   1 +
>  configure.ac                 |   1 +
>  wpan-hwsim/Makefile.am       |   8 +
>  wpan-hwsim/mac802154_hwsim.h |  73 +++++++
>  wpan-hwsim/wpan-hwsim.c      | 509 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 592 insertions(+)
>  create mode 100644 wpan-hwsim/Makefile.am
>  create mode 100644 wpan-hwsim/mac802154_hwsim.h
>  create mode 100644 wpan-hwsim/wpan-hwsim.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 37f18b6..3f15825 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -21,4 +21,5 @@ AM_LDFLAGS = \
>  SUBDIRS = \
>  	src \
>  	wpan-ping \
> +	wpan-hwsim \
>  	examples
> diff --git a/configure.ac b/configure.ac
> index 5dd59dc..a983532 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -56,6 +56,7 @@ AC_CONFIG_FILES([
>          Makefile
>  	src/Makefile
>  	wpan-ping/Makefile
> +	wpan-hwsim/Makefile
>  	examples/Makefile
>  ])
>  
> diff --git a/wpan-hwsim/Makefile.am b/wpan-hwsim/Makefile.am
> new file mode 100644
> index 0000000..f688cd9
> --- /dev/null
> +++ b/wpan-hwsim/Makefile.am
> @@ -0,0 +1,8 @@
> +bin_PROGRAMS = wpan-hwsim
> +
> +wpan_hwsim_SOURCES = wpan-hwsim.c
> +
> +wpan_hwsim_CFLAGS = $(AM_CFLAGS) $(LIBNL3_CFLAGS)
> +wpan_hwsim_LDADD = $(LIBNL3_LIBS)
> +
> +EXTRA_DIST = README.wpan-hwsim

This patch does not have any README.wpan-ping file. Did you miss to add
it? Having a basic description in a README would be helpful for starters.

> diff --git a/wpan-hwsim/mac802154_hwsim.h b/wpan-hwsim/mac802154_hwsim.h
> new file mode 100644
> index 0000000..6009214
> --- /dev/null
> +++ b/wpan-hwsim/mac802154_hwsim.h
> @@ -0,0 +1,73 @@
> +#ifndef __MAC802154_HWSIM_H
> +#define __MAC802154_HWSIM_H
> +
> +/* mac802154 hwsim netlink commands
> + *
> + * @MAC802154_HWSIM_CMD_UNSPEC: unspecified command to catch error
> + * @MAC802154_HWSIM_CMD_GET_RADIO: fetch information about existing radios
> + * @MAC802154_HWSIM_CMD_SEL_RADIO: change radio parameters during runtime

Typo. SET_RADIO.

> + * @MAC802154_HWSIM_CMD_NEW_RADIO: create a new radio with the given parameters
> + *	returns the radio ID (>= 0) or negative on errors, if successful
> + *	then multicast the result
> + * @MAC802154_HWSIM_CMD_DEL_RADIO: destroy a radio, reply is multicasted
> + * @MAC802154_HWSIM_CMD_GET_EDGE: fetch information about existing edges
> + * @MAC802154_HWSIM_CMD_SET_EDGE: change edge parameters during runtime
> + * @MAC802154_HWSIM_CMD_DEL_EDGE: delete edges between radios
> + * @MAC802154_HWSIM_CMD_NEW_EDGE: create a new edge between two radios
> + * @__MAC802154_HWSIM_CMD_MAX: enum limit
> + */
> +enum {
> +	MAC802154_HWSIM_CMD_UNSPEC,
> +
> +	MAC802154_HWSIM_CMD_GET_RADIO,
> +	MAC802154_HWSIM_CMD_SET_RADIO,
> +	MAC802154_HWSIM_CMD_NEW_RADIO,
> +	MAC802154_HWSIM_CMD_DEL_RADIO,
> +
> +	MAC802154_HWSIM_CMD_GET_EDGE,
> +	MAC802154_HWSIM_CMD_SET_EDGE,
> +	MAC802154_HWSIM_CMD_DEL_EDGE,
> +	MAC802154_HWSIM_CMD_NEW_EDGE,
> +
> +	__MAC802154_HWSIM_CMD_MAX,
> +};
> +
> +#define MAC802154_HWSIM_CMD_MAX (__MAC802154_HWSIM_MAX - 1)
> +
> +/* mac802154 hwsim netlink attributes
> + *
> + * @MAC802154_HWSIM_ATTR_UNSPEC: unspecified attribute to catch error
> + * @MAC802154_HWSIM_ATTR_RADIO_ID: u32 attribute to identify the radio
> + * @MAC802154_HWSIM_ATTR_EDGE: nested attribute of edges
> + * @MAC802154_HWSIM_ATTR_EDGES: list if nested attributes which contains the
> + *	edge information according the radio id




> + * @__MAC802154_HWSIM_ATTR_MAX: enum limit
> + */
> +enum {
> +	MAC802154_HWSIM_ATTR_UNSPEC,
> +	MAC802154_HWSIM_ATTR_RADIO_ID,
> +	MAC802154_HWSIM_ATTR_RADIO_EDGE,
> +	MAC802154_HWSIM_ATTR_RADIO_EDGES,
> +	__MAC802154_HWSIM_ATTR_MAX,
> +};
> +
> +#define MAC802154_HWSIM_ATTR_MAX (__MAC802154_HWSIM_ATTR_MAX - 1)
> +
> +/* mac802154 hwsim edge netlink attributes
> + *
> + * @MAC802154_HWSIM_EDGE_ATTR_UNSPEC: unspecified attribute to catch error
> + * @MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID: radio id where the edge points to
> + * @MAC802154_HWSIM_EDGE_ATTR_LQI: LQI value which the endpoint radio will
> + *	receive for this edge
> + * @__MAC802154_HWSIM_ATTR_MAX: enum limit
> + */
> +enum {
> +	MAC802154_HWSIM_EDGE_ATTR_UNSPEC,
> +	MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID,
> +	MAC802154_HWSIM_EDGE_ATTR_LQI,
> +	__MAC802154_HWSIM_EDGE_ATTR_MAX,
> +};
> +
> +#define MAC802154_HWSIM_EDGE_ATTR_MAX (__MAC802154_HWSIM_EDGE_ATTR_MAX - 1)
> +
> +#endif /* __MAC802154_HWSIM_H */
> diff --git a/wpan-hwsim/wpan-hwsim.c b/wpan-hwsim/wpan-hwsim.c
> new file mode 100644
> index 0000000..2d74e99
> --- /dev/null
> +++ b/wpan-hwsim/wpan-hwsim.c
> @@ -0,0 +1,509 @@
> +/*
> + * Linux IEEE 802.15.4 hwsim tool
> + *
> + * Copyright (C) 2018 Alexander Aring <aring@xxxxxxxxxxxx>
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <inttypes.h>
> +#include <stdbool.h>
> +#include <getopt.h>
> +#include <stdint.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <stdio.h>
> +
> +#include <netlink/netlink.h>
> +
> +#include <netlink/genl/genl.h>
> +#include <netlink/genl/family.h>
> +#include <netlink/genl/ctrl.h>
> +
> +#include "mac802154_hwsim.h"
> +
> +static struct nl_sock *nl_sock;
> +static int nlhwsim_id;
> +
> +static int nlhwsim_init(void)
> +{
> +	int err;
> +
> +	nl_sock = nl_socket_alloc();
> +	if (!nl_sock) {
> +		fprintf(stderr, "Failed to allocate netlink socket.\n");
> +		return -ENOMEM;
> +	}
> +
> +	nl_socket_set_buffer_size(nl_sock, 8192, 8192);
> +
> +	if (genl_connect(nl_sock)) {
> +		fprintf(stderr, "Failed to connect to generic netlink.\n");
> +		err = -ENOLINK;
> +		goto out_handle_destroy;
> +	}
> +
> +	nlhwsim_id = genl_ctrl_resolve(nl_sock, "MAC802154_HWSIM");
> +	if (nlhwsim_id < 0) {
> +		fprintf(stderr, "MAC802154_HWSIM not found.\n");
> +		err = -ENOENT;
> +		nl_close(nl_sock);
> +		goto out_handle_destroy;
> +	}
> +
> +	return 0;
> +
> +out_handle_destroy:
> +	nl_socket_free(nl_sock);
> +	return err;
> +}
> +
> +static void nlhwsim_cleanup(void)
> +{
> +	nl_close(nl_sock);
> +	nl_socket_free(nl_sock);
> +}
> +
> +static int error_handler(struct sockaddr_nl *nla, struct nlmsgerr *err,
> +			 void *arg)
> +{
> +	int *ret = arg;
> +	*ret = err->error;
> +	return NL_STOP;
> +}
> +
> +static int hwsim_create(void)
> +{
> +	struct nl_msg *msg;
> +	struct nl_cb *cb;
> +	int idx = 0, ret;
> +
> +	cb = nl_cb_alloc(NL_CB_DEFAULT);
> +	if (!cb)
> +		return -ENOMEM;
> +
> +	msg = nlmsg_alloc();
> +	if (!cb) {

You want to check on msg here and not cb. It would never reach the if
!cb. I think this is one of the problems Coverity pointed out above.

> +		nl_cb_put(cb);
> +		return -ENOMEM;
> +	}
> +
> +	genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, nlhwsim_id, 0, 0,
> +		    MAC802154_HWSIM_CMD_NEW_RADIO, 0);
> +	ret = nl_send_auto(nl_sock, msg);
> +	if (ret < 0) {
> +		nl_cb_put(cb);
> +		return ret;
> +	}
> +
> +	nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &idx);
> +	nl_recvmsgs(nl_sock, cb);
> +	printf("wpan_hwsim radio%d registered.\n", idx);
> +
> +	nl_cb_put(cb);
> +
> +	return 0;
> +}
> +
> +static int nl_msg_dot_cb(struct nl_msg* msg, void* arg)
> +{
> +	static struct nla_policy edge_policy[MAC802154_HWSIM_EDGE_ATTR_MAX + 1] = {
> +		[MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID] = { .type = NLA_U32 },
> +		[MAC802154_HWSIM_EDGE_ATTR_LQI] = { .type = NLA_U8 },
> +	};
> +	struct nlmsghdr *nlh = nlmsg_hdr(msg);
> +	struct genlmsghdr *gnlh = (struct genlmsghdr*) nlmsg_data(nlh);
> +	struct nlattr *attrs[MAC802154_HWSIM_ATTR_MAX + 1];
> +	struct nlattr *edge[MAC802154_HWSIM_EDGE_ATTR_MAX + 1];
> +	unsigned int *ignore_idx = arg;
> +	struct nlattr *nl_edge;
> +	uint32_t idx, idx2;
> +	uint8_t lqi;
> +	int rem_edge;
> +	int rc;
> +
> +	nla_parse(attrs, MAC802154_HWSIM_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
> +		  genlmsg_attrlen(gnlh, 0), NULL);
> +
> +	if (!attrs[MAC802154_HWSIM_ATTR_RADIO_ID])
> +		return NL_SKIP;
> +
> +	idx = nla_get_u32(attrs[MAC802154_HWSIM_ATTR_RADIO_ID]);
> +	if (idx == *ignore_idx)
> +		return NL_SKIP;
> +
> +	if (!attrs[MAC802154_HWSIM_ATTR_RADIO_EDGES])
> +		return NL_SKIP;
> +
> +	nla_for_each_nested(nl_edge, attrs[MAC802154_HWSIM_ATTR_RADIO_EDGES],
> +			    rem_edge) {
> +		rc = nla_parse_nested(edge, MAC802154_HWSIM_EDGE_ATTR_MAX,
> +				      nl_edge, edge_policy);
> +		if (rc)
> +			return NL_SKIP;
> +
> +		idx2 = nla_get_u32(edge[MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID]);
> +		if (idx2 == *ignore_idx)
> +			continue;
> +
> +		lqi = nla_get_u32(edge[MAC802154_HWSIM_EDGE_ATTR_LQI]);
> +		printf("\t%" PRIu32 " -> %" PRIu32 "[label=%" PRIu8 "];\n",
> +		       idx, idx2, lqi);
> +	}
> +
> +	return NL_SKIP;
> +}
> +
> +static int nl_msg_cb(struct nl_msg* msg, void* arg)
> +{
> +	static struct nla_policy edge_policy[MAC802154_HWSIM_EDGE_ATTR_MAX + 1] = {
> +		[MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID] = { .type = NLA_U32 },
> +		[MAC802154_HWSIM_EDGE_ATTR_LQI] = { .type = NLA_U8 },
> +	};
> +	struct nlmsghdr *nlh = nlmsg_hdr(msg);
> +	struct genlmsghdr *gnlh = (struct genlmsghdr*) nlmsg_data(nlh);
> +	struct nlattr *attrs[MAC802154_HWSIM_ATTR_MAX + 1];
> +	struct nlattr *edge[MAC802154_HWSIM_EDGE_ATTR_MAX + 1];
> +	unsigned int *ignore_idx = arg;
> +	struct nlattr *nl_edge;
> +	uint32_t idx;
> +	uint8_t lqi;
> +	int rem_edge;
> +	int rc;
> +
> +	nla_parse(attrs, MAC802154_HWSIM_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
> +		  genlmsg_attrlen(gnlh, 0), NULL);
> +
> +	if (!attrs[MAC802154_HWSIM_ATTR_RADIO_ID])
> +		return NL_SKIP;
> +
> +	idx = nla_get_u32(attrs[MAC802154_HWSIM_ATTR_RADIO_ID]);
> +
> +	if (idx == *ignore_idx)
> +		return NL_SKIP;
> +
> +	printf("wpan_hwsim radio%" PRIu32 ":\n", idx);
> +
> +	if (!attrs[MAC802154_HWSIM_ATTR_RADIO_EDGES])
> +		return NL_SKIP;
> +
> +	nla_for_each_nested(nl_edge, attrs[MAC802154_HWSIM_ATTR_RADIO_EDGES],
> +			    rem_edge) {
> +		rc = nla_parse_nested(edge, MAC802154_HWSIM_EDGE_ATTR_MAX,
> +				      nl_edge, edge_policy);
> +		if (rc)
> +			return NL_SKIP;
> +
> +		idx = nla_get_u32(edge[MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID]);
> +
> +		if (idx == *ignore_idx)
> +			continue;
> +
> +		printf("\tedge:\n");
> +		printf("\t\tradio%" PRIu32 "\n", idx);
> +		lqi = nla_get_u32(edge[MAC802154_HWSIM_EDGE_ATTR_LQI]);
> +		printf("\t\tlqi: 0x%02x\n", lqi);
> +	}
> +
> +	return NL_SKIP;
> +}
> +
> +static int hwsim_dump(bool dot, int unsigned ignore_idx)
> +{
> +	struct nl_msg *msg;
> +	int rc;
> +
> +	nl_socket_modify_cb(nl_sock, NL_CB_VALID, NL_CB_CUSTOM,
> +			    dot ? nl_msg_dot_cb : nl_msg_cb, &ignore_idx);
> +	msg = nlmsg_alloc();
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, nlhwsim_id, 0, NLM_F_DUMP,
> +		    MAC802154_HWSIM_CMD_GET_RADIO, 0);
> +
> +	if (dot)
> +		printf("digraph {\n");
> +
> +	rc = nl_send_sync(nl_sock, msg);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (dot)
> +		printf("}\n");
> +
> +	return rc;
> +}
> +
> +static int hwsim_del(uint32_t idx)
> +{
> +	struct nl_msg *msg;
> +	struct nl_cb *cb;
> +	int err = 0;
> +	int rc;
> +
> +	cb = nl_cb_alloc(NL_CB_DEFAULT);
> +	if (!cb)
> +		return -ENOMEM;
> +
> +	msg = nlmsg_alloc();
> +	if (!msg) {
> +		nl_cb_put(cb);
> +		return -ENOMEM;
> +	}
> +
> +	genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, nlhwsim_id, 0, 0,
> +		    MAC802154_HWSIM_CMD_DEL_RADIO, 0);
> +	nla_put_u32(msg, MAC802154_HWSIM_ATTR_RADIO_ID, idx);
> +
> +	nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &err);
> +	rc = nl_send_auto(nl_sock, msg);
> +	nl_recvmsgs(nl_sock, cb);
> +	if (err < 0)
> +		fprintf(stderr, "Failed to remove radio: %s\n", strerror(abs(err)));
> +	nl_cb_put(cb);
> +
> +	return rc;
> +}
> +
> +static int hwsim_cmd_edge(int cmd, uint32_t idx, uint32_t idx2, uint8_t lqi)
> +{
> +	struct nlattr* edge;
> +	struct nl_msg *msg;
> +	struct nl_cb *cb;
> +	int err = 0;
> +	int rc;
> +
> +	cb = nl_cb_alloc(NL_CB_DEFAULT);
> +	if (!cb)
> +		return -ENOMEM;
> +
> +	msg = nlmsg_alloc();
> +	if (!msg) {
> +		nl_cb_put(cb);
> +		return -ENOMEM;
> +	}
> +
> +	genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, nlhwsim_id, 0, 0,
> +		    cmd, 0);
> +	nla_put_u32(msg, MAC802154_HWSIM_ATTR_RADIO_ID, idx);
> +
> +	edge = nla_nest_start(msg, MAC802154_HWSIM_ATTR_RADIO_EDGE);
> +	if (!edge) {
> +		nl_cb_put(cb);
> +		return -ENOMEM;
> +	}
> +
> +	nla_put_u32(msg, MAC802154_HWSIM_EDGE_ATTR_ENDPOINT_ID, idx2);
> +	if (cmd == MAC802154_HWSIM_CMD_SET_EDGE)
> +		nla_put_u8(msg, MAC802154_HWSIM_EDGE_ATTR_LQI, lqi);
> +
> +	nla_nest_end(msg, edge);
> +
> +	nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &err);
> +	rc = nl_send_auto(nl_sock, msg);
> +	nl_recvmsgs(nl_sock, cb);
> +	if (err < 0)
> +		fprintf(stderr, "Failed to add or remove edge: %s\n", strerror(abs(err)));
> +	nl_cb_put(cb);
> +
> +	return rc;
> +}
> +
> +static int hwsim_do_cmd(uint32_t cmd, unsigned int idx, unsigned int idx2,
> +			uint8_t lqi, bool dot, unsigned int ignore_idx)
> +{
> +	int rc;
> +
> +	rc = nlhwsim_init();
> +	if (rc)
> +		return 1;
> +
> +	switch (cmd) {
> +	case MAC802154_HWSIM_CMD_GET_RADIO:
> +		rc = hwsim_dump(dot, ignore_idx);
> +		break;
> +	case MAC802154_HWSIM_CMD_DEL_RADIO:
> +		rc = hwsim_del(idx);
> +		break;
> +	case MAC802154_HWSIM_CMD_NEW_RADIO:
> +		rc = hwsim_create();
> +		break;
> +	case MAC802154_HWSIM_CMD_NEW_EDGE:
> +	case MAC802154_HWSIM_CMD_DEL_EDGE:
> +	case MAC802154_HWSIM_CMD_SET_EDGE:
> +		rc = hwsim_cmd_edge(cmd, idx, idx2, lqi);
> +		break;

For AC802154_HWSIM_CMD_SET_RADIO and MAC802154_HWSIM_CMD_GET_EDGE we
have commands in the netlink API but no users in this tool. Which makes
me wonder if we need them.


> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	nlhwsim_cleanup();
> +
> +	return rc;
> +}
> +
> +static void print_usage(void)
> +{
> +	printf("wpan_hwsim [OPTION...] [CMD...]\n");
> +	printf("\n");
> +	printf(" possible options:\n");
> +	printf("  -h, --help             show this help\n");
> +	printf("  -v, --version          show version\n");
> +	printf("  -d, --dot              dump topology as dot format\n");
> +	printf("  -i, --ignore           filter one node from dump (useful for virtual monitors)\n");
> +	printf("\n");
> +	printf(" possible commands:\n");
> +	printf("  add                    add new hwsim radio\n");
> +	printf("  del IDX                del hwsim radio according idx\n");
> +	printf("  edge add IDX IDX       add edge between radios\n");
> +	printf("  edge del IDX IDX       delete edge between radios\n");
> +	printf("  edge lqi IDX IDX LQI   set lqi value for a specific edge\n");
> +	printf("\n");
> +	printf("  To dump all node information just call this program without any command\n");
> +}
> +
> +static void print_version(void)
> +{
> +	printf("0.1\n");

Printing PACKAGE_VERSION here would be better. To have the version in
sync with the other binaries.

> +}
> +
> +int main(int argc, const char *argv[])
> +{
> +	unsigned long int idx, idx2, lqi, ignore_idx = ULONG_MAX;
> +	bool dot = false;
> +	int cmd;
> +	int rc;
> +	int c;
> +
> +	while (1) {
> +		int option_index = 0;
> +		static struct option long_options[] = {
> +			{"help",	required_argument,	0,	'h' },
> +			{"version",	required_argument,	0,	'v' },
> +			{"dot",		required_argument,	0,	'd' },
> +			{"ignore",	required_argument,	0,	'i' },
> +			{0,				0,	0,	0 }
> +		};
> +
> +		c = getopt_long(argc, (char **)argv, "vhdi:",
> +				long_options, &option_index);
> +		if (c == -1)
> +			break;
> +
> +		switch (c) {
> +		case 'v':
> +			print_version();
> +			return EXIT_SUCCESS;
> +		case 'h':
> +			print_usage();
> +			return EXIT_SUCCESS;
> +		case 'd':
> +			dot = true;
> +			break;
> +		case 'i':
> +			ignore_idx = strtoul(optarg, NULL, 0);
> +			if (ignore_idx == ULONG_MAX) {
> +				fprintf(stderr, "Invalid radio index for ignore argument\n");
> +				return EXIT_FAILURE;
> +			}
> +			break;
> +		default:
> +			print_usage();
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
> +	if (optind + 1 > argc) {
> +		rc = hwsim_do_cmd(MAC802154_HWSIM_CMD_GET_RADIO, 0, 0, 0, dot,
> +				  ignore_idx);
> +		if (rc < 0)
> +			return EXIT_FAILURE;
> +		else
> +			goto out;
> +	}
> +
> +	if (!strncmp(argv[optind], "add", 3)) {
> +		rc = hwsim_do_cmd(MAC802154_HWSIM_CMD_NEW_RADIO, 0, 0, 0, 0, 0);
> +		if (rc < 0)
> +			return EXIT_FAILURE;
> +	} else if (!strncmp(argv[optind], "del", 3)) {
> +		if (optind + 2 > argc) {
> +			fprintf(stderr, "Missing radio index for delete\n");
> +			return EXIT_FAILURE;
> +		} else {
> +			idx = strtoul(argv[optind + 1], NULL, 0);
> +			if (idx == ULONG_MAX) {
> +				fprintf(stderr, "Invalid radio index for delete\n");
> +				return EXIT_FAILURE;
> +			}
> +
> +			rc = hwsim_do_cmd(MAC802154_HWSIM_CMD_DEL_RADIO, idx, 0, 0, 0, 0);
> +			if (rc < 0)
> +				return EXIT_FAILURE;
> +		}
> +	} else if (!strncmp(argv[optind], "edge", 4)) {
> +		if (optind + 4 > argc) {
> +			fprintf(stderr, "Missing edge radio index information\n");
> +			return EXIT_FAILURE;
> +		} else {
> +			if (!strncmp(argv[optind + 1], "add", 3)) {
> +				cmd = MAC802154_HWSIM_CMD_NEW_EDGE;
> +			} else if (!strncmp(argv[optind + 1], "del", 3)) {
> +				cmd = MAC802154_HWSIM_CMD_DEL_EDGE;
> +			} else if (!strncmp(argv[optind + 1], "lqi", 3)) {
> +				cmd = MAC802154_HWSIM_CMD_SET_EDGE;
> +			} else {
> +				fprintf(stderr, "Invalid edge command\n");
> +				return EXIT_FAILURE;
> +			}
> +
> +			if (cmd == MAC802154_HWSIM_CMD_SET_EDGE) {
> +				if (optind + 5 > argc) {
> +					fprintf(stderr, "LQI information missing\n");
> +					return EXIT_FAILURE;
> +				}
> +
> +				lqi = strtoul(argv[optind + 4], NULL, 0);
> +				if (lqi == ULONG_MAX || lqi > 0xff || lqi < 0) {
> +					fprintf(stderr, "Invalid lqi value\n");
> +					return EXIT_FAILURE;
> +				}
> +			}
> +
> +			idx = strtoul(argv[optind + 2], NULL, 0);
> +			if (idx == ULONG_MAX) {
> +				fprintf(stderr, "Invalid first radio index for edge command\n");
> +				return EXIT_FAILURE;
> +			}
> +
> +			idx2 = strtoul(argv[optind + 3], NULL, 0);
> +			if (idx2 == ULONG_MAX) {
> +				fprintf(stderr, "Invalid second radio index for edge command\n");
> +				return EXIT_FAILURE;
> +			}
> +
> +			rc = hwsim_do_cmd(cmd, idx, idx2, lqi, 0, 0);
> +			if (rc < 0)
> +				return EXIT_FAILURE;
> +		}
> +	} else {
> +		fprintf(stderr, "Unknown command\n");
> +		print_usage();
> +		return EXIT_FAILURE;
> +	}
> +
> +out:
> +	return EXIT_SUCCESS;
> +}
> 

The rest looks fine. I still have to review the kernel part.

regards
Stefan Schmidt
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux