Hi Hoang,
Both patches are fine with me, apart from comments below. Just send it out to
tipc-discussion so Ying can have a look, too.
BR
///jon
> -----Original Message-----
> From: Hoang Le <hoang.h...@dektech.com.au>
> Sent: September 27, 2018 5:11 AM
> To: tipc-discussion@lists.sourceforge.net; Jon Maloy
> <jon.ma...@ericsson.com>; ma...@donjonn.com; ying....@windriver.com
> Subject: [iproute2-next] tipc:support for indicating interface when activating
> UDP bearer
You need a commit log message in both patches that briefly explains what this
is all about.
>
> Signed-off-by: Hoang Le <hoang.h...@dektech.com.au>
> ---
> tipc/bearer.c | 82 ++++++++++++++++++++++++++++++++++++++++++++-
> ------
> 1 file changed, 72 insertions(+), 10 deletions(-)
>
> diff --git a/tipc/bearer.c b/tipc/bearer.c index 05dc84aa8ded..25bc85be980d
> 100644
> --- a/tipc/bearer.c
> +++ b/tipc/bearer.c
> @@ -19,6 +19,8 @@
> #include <linux/tipc_netlink.h>
> #include <linux/tipc.h>
> #include <linux/genetlink.h>
> +#include <linux/if.h>
> +#include <sys/ioctl.h>
>
> #include <libmnl/libmnl.h>
> #include <sys/socket.h>
> @@ -68,7 +70,7 @@ static void cmd_bearer_enable_l2_help(struct cmdl
> *cmdl, char *media) static void cmd_bearer_enable_udp_help(struct cmdl
> *cmdl, char *media) {
> fprintf(stderr,
> - "Usage: %s bearer enable [OPTIONS] media %s name NAME
> localip IP [UDP OPTIONS]\n\n",
> + "Usage: %s bearer enable [OPTIONS] media %s [name NAME
> localip
> +IP|device DEVICE] [UDP OPTIONS]\n\n",
> cmdl->argv[0], media);
> fprintf(stderr,
> "OPTIONS\n"
> @@ -121,6 +123,48 @@ static int generate_multicast(short af, char *buf, int
> bufsize)
> return 0;
> }
>
> +static int cmd_bearer_validate_and_get_addr(const char *name, char
> +*straddr) {
> + struct ifreq ifc;
> + struct sockaddr_in *ip4addr;
> + struct sockaddr_in6 *ip6addr;
> + int fd = 0;
> +
> + if (!name || !straddr)
> + return 0;
> +
> + fd = socket(PF_INET, SOCK_DGRAM, 0);
> + if (fd <= 0) {
> + fprintf(stderr, "Failed to create socket\n");
> + return 0;
> + }
> +
> + ifc.ifr_name[0] = 0;
> + memcpy(ifc.ifr_name, name, strlen(name));
> + ifc.ifr_name[strlen(name)] = 0;
> +
> + if (ioctl(fd, SIOCGIFADDR, &ifc) < 0) {
> + fprintf(stderr, "ioctl failed :%s\n", strerror(errno));
> + close(fd);
> + return 0;
> + }
> +
> + ip4addr = (struct sockaddr_in *)&ifc.ifr_addr;
> + if (inet_ntop(AF_INET, &ip4addr->sin_addr, straddr,
> + INET_ADDRSTRLEN) == NULL)
> + {
> + ip6addr = (struct sockaddr_in6 *)&ifc.ifr_addr;
> + if (inet_ntop(AF_INET6, &ip6addr->sin6_addr, straddr,
> + INET6_ADDRSTRLEN) == NULL)
> + {
> + fprintf(stderr, "UDP local address error\n");
> + return -EINVAL;
> + }
> + }
> + close(fd);
> + return 1;
> +}
> +
> static int nl_add_udp_enable_opts(struct nlmsghdr *nlh, struct opt *opts,
> struct cmdl *cmdl)
> {
> @@ -138,13 +182,27 @@ static int nl_add_udp_enable_opts(struct nlmsghdr
> *nlh, struct opt *opts,
> .ai_family = AF_UNSPEC,
> .ai_socktype = SOCK_DGRAM
> };
> + char addr[INET6_ADDRSTRLEN] = {0};
>
> - if (!(opt = get_opt(opts, "localip"))) {
> - fprintf(stderr, "error, udp bearer localip missing\n");
> - cmd_bearer_enable_udp_help(cmdl, "udp");
> - return -EINVAL;
> + opt = get_opt(opts, "device");
> + if (opt) {
> + if (!cmd_bearer_validate_and_get_addr(opt->val, addr)) {
> + fprintf(stderr, "error, no device name available\n");
> + return -EINVAL;
> + }
> + }
To avoid unnecessarily nested if() clauses you could just do
If (opt && !cmd_bearer_validate_and_get_addr(opt->val, addr))
{
}
> +
> + if (strlen(addr) > 0)
> + locip = addr;
> +
We follow kernel coding style, so if you use the curly brackets in one clause
you have to do it in all of them.
if() {
} else {
}
you have
else {
> + opt = get_opt(opts, "localip");
> + if (!opt) {
> + fprintf(stderr, "error, udp bearer localip missing\n");
> + cmd_bearer_enable_udp_help(cmdl, "udp");
> + return -EINVAL;
> + }
> + locip = opt->val;
> }
> - locip = opt->val;
>
> if ((opt = get_opt(opts, "remoteip")))
> remip = opt->val;
> @@ -262,13 +320,13 @@ int cmd_get_unique_bearer_name(const struct
> cmd *cmd, struct cmdl *cmdl,
>
> static void cmd_bearer_add_udp_help(struct cmdl *cmdl, char *media) {
> - fprintf(stderr, "Usage: %s bearer add media %s name NAME
> remoteip REMOTEIP\n\n",
> + fprintf(stderr, "Usage: %s bearer add media %s [name NAME|device
> +DEVICE]\n\nremoteip REMOTEIP\n\n",
> cmdl->argv[0], media);
> }
>
> static void cmd_bearer_add_help(struct cmdl *cmdl) {
> - fprintf(stderr, "Usage: %s bearer add media udp name NAME
> remoteip REMOTEIP\n",
> + fprintf(stderr, "Usage: %s bearer add media udp [name
> NAME|device
> +DEVICE] remoteip REMOTEIP\n",
> cmdl->argv[0]);
> }
>
> @@ -325,6 +383,7 @@ static int cmd_bearer_add_media(struct nlmsghdr
> *nlh, const struct cmd *cmd,
> { "remoteport", OPT_KEYVAL, NULL },
> { "name", OPT_KEYVAL, NULL },
> { "media", OPT_KEYVAL, NULL },
> + { "device", OPT_KEYVAL, NULL },
> { NULL }
> };
> const struct tipc_sup_media sup_media[] = { @@ -349,8 +408,10 @@
> static int cmd_bearer_add_media(struct nlmsghdr *nlh, const struct cmd
> *cmd,
> return -EINVAL;
> }
> if (!(opt = get_opt(opts, "name"))) {
> - fprintf(stderr, "error, missing media name\n");
> - return -EINVAL;
> + if (!(opt = get_opt(opts, "device"))) {
> + fprintf(stderr, "error, missing media name\n");
> + return -EINVAL;
> + }
> }
>
> if (!(nlh = msg_init(buf, TIPC_NL_BEARER_ADD))) { @@ -557,6 +618,7
> @@ static int cmd_bearer_set_prop(struct nlmsghdr *nlh, const struct cmd
> *cmd,
> };
> struct tipc_sup_media sup_media[] = {
> { "udp", "name", cmd_bearer_set_udp_help},
> + { "udp", "device", cmd_bearer_set_udp_help},
> { "eth", "device", cmd_bearer_set_l2_help },
> { "ib", "device", cmd_bearer_set_l2_help },
> { NULL, },
> --
> 2.17.1
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
