From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Steve Wise" Subject: RE: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands Date: Wed, 27 Feb 2019 15:18:17 -0600 Message-ID: <021e01d4cee1$f54acaf0$dfe060d0$@opengridcomputing.com> References: <5dd9d9aeada48bc5db0eb0394ed4e3ce38ee41bc.1550773362.git.swise@opengridcomputing.com> <20190223094311.GP23561@mtr-leonro.mtl.com> <94c6a44c-6f08-ffe8-9798-a4faf14bd29a@opengridcomputing.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <94c6a44c-6f08-ffe8-9798-a4faf14bd29a@opengridcomputing.com> Content-Language: en-us Sender: netdev-owner@vger.kernel.org To: 'Leon Romanovsky' Cc: dsahern@gmail.com, stephen@networkplumber.org, netdev@vger.kernel.org, linux-rdma@vger.kernel.org List-Id: linux-rdma@vger.kernel.org > -----Original Message----- > From: Steve Wise > Sent: Tuesday, February 26, 2019 11:25 AM > To: Leon Romanovsky > Cc: dsahern@gmail.com; stephen@networkplumber.org; > netdev@vger.kernel.org; linux-rdma@vger.kernel.org > Subject: Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' > commands > > > On 2/23/2019 3:43 AM, Leon Romanovsky wrote: > > On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote: > >> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft- > rdma > >> device to a netdev interface. > >> > >> EG: > >> > >> rdma link add rxe_eth0 type rxe netdev eth0 > >> rdma link delete rxe_eth0 > >> > >> Signed-off-by: Steve Wise > >> --- > >> rdma/link.c | 67 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> rdma/rdma.h | 1 + > >> rdma/utils.c | 2 +- > >> 3 files changed, 69 insertions(+), 1 deletion(-) > >> > >> diff --git a/rdma/link.c b/rdma/link.c > >> index c064be627be2..afaf19663728 100644 > >> --- a/rdma/link.c > >> +++ b/rdma/link.c > >> @@ -14,6 +14,9 @@ > >> static int link_help(struct rd *rd) > >> { > >> pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename); > >> + pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n", > >> + rd->filename); > >> + pr_out("Usage: %s link delete NAME\n", rd->filename); > >> return 0; > >> } > >> > >> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd) > >> return rd_exec_link(rd, link_one_show, true); > >> } > >> > >> +static int link_add(struct rd *rd) > >> +{ > >> + char *name; > >> + char *type = NULL; > >> + char *dev = NULL; > >> + uint32_t seq; > >> + > >> + if (rd_no_arg(rd)) { > >> + pr_err("No link name was supplied\n"); > > I think that it is better to have instruction message and not error > > message: "Please provide ...". > > > Ok.  Perhaps a new utility rd_exec_require_link() can be created? > > > >> + return -EINVAL; > >> + } > >> + name = rd_argv(rd); > >> + rd_arg_inc(rd); > >> + while (!rd_no_arg(rd)) { > >> + if (rd_argv_match(rd, "type")) { > >> + rd_arg_inc(rd); > >> + type = rd_argv(rd); > >> + } else if (rd_argv_match(rd, "netdev")) { > >> + rd_arg_inc(rd); > >> + dev = rd_argv(rd); > >> + } else { > >> + pr_err("Invalid parameter %s\n", rd_argv(rd)); > >> + return -EINVAL; > >> + } > >> + rd_arg_inc(rd); > > Please use chains of struct rd_cmd and rd_exec_cmd() instead of > > open-coding parser. > > > Ok.  Like your recently merged series did... > > > >> + } > >> + if (!type) { > >> + pr_err("No type was supplied\n"); > >> + return -EINVAL; > > General parser handle it. > > > Ok. > > > >> + } > >> + if (!dev) { > >> + pr_err("No net device was supplied\n"); > >> + return -EINVAL; > >> + } > > rd_exec_require_dev() ??? > > > Looks like I can use that.  I'll try it. Actually, in the above code, the variable 'dev' is the netdev string, and that is the 3rd parameter to the 'link add' command, so it isn't appropriate for rd_exec_require_dev(). Steve From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 24269C43381 for ; Wed, 27 Feb 2019 21:18:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F18C32083D for ; Wed, 27 Feb 2019 21:18:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730393AbfB0VSU convert rfc822-to-8bit (ORCPT ); Wed, 27 Feb 2019 16:18:20 -0500 Received: from linode.aoot.com ([69.164.194.13]:49004 "EHLO linode.aoot.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727488AbfB0VSU (ORCPT ); Wed, 27 Feb 2019 16:18:20 -0500 Received: from stevoacer (47-221-137-213.gtwncmta03.res.dyn.suddenlink.net [47.221.137.213]) by linode.aoot.com (Postfix) with ESMTP id 23193821C; Wed, 27 Feb 2019 15:18:19 -0600 (CST) From: "Steve Wise" To: "'Leon Romanovsky'" Cc: , , , References: <5dd9d9aeada48bc5db0eb0394ed4e3ce38ee41bc.1550773362.git.swise@opengridcomputing.com> <20190223094311.GP23561@mtr-leonro.mtl.com> <94c6a44c-6f08-ffe8-9798-a4faf14bd29a@opengridcomputing.com> In-Reply-To: <94c6a44c-6f08-ffe8-9798-a4faf14bd29a@opengridcomputing.com> Subject: RE: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands Date: Wed, 27 Feb 2019 15:18:17 -0600 Message-ID: <021e01d4cee1$f54acaf0$dfe060d0$@opengridcomputing.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQII7r6I2VyJPIdQNc8sid040CTlwALHEUnSAtgSaLUBvRD97KVRBGkA Content-Language: en-us Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org > -----Original Message----- > From: Steve Wise > Sent: Tuesday, February 26, 2019 11:25 AM > To: Leon Romanovsky > Cc: dsahern@gmail.com; stephen@networkplumber.org; > netdev@vger.kernel.org; linux-rdma@vger.kernel.org > Subject: Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' > commands > > > On 2/23/2019 3:43 AM, Leon Romanovsky wrote: > > On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote: > >> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft- > rdma > >> device to a netdev interface. > >> > >> EG: > >> > >> rdma link add rxe_eth0 type rxe netdev eth0 > >> rdma link delete rxe_eth0 > >> > >> Signed-off-by: Steve Wise > >> --- > >> rdma/link.c | 67 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> rdma/rdma.h | 1 + > >> rdma/utils.c | 2 +- > >> 3 files changed, 69 insertions(+), 1 deletion(-) > >> > >> diff --git a/rdma/link.c b/rdma/link.c > >> index c064be627be2..afaf19663728 100644 > >> --- a/rdma/link.c > >> +++ b/rdma/link.c > >> @@ -14,6 +14,9 @@ > >> static int link_help(struct rd *rd) > >> { > >> pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename); > >> + pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n", > >> + rd->filename); > >> + pr_out("Usage: %s link delete NAME\n", rd->filename); > >> return 0; > >> } > >> > >> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd) > >> return rd_exec_link(rd, link_one_show, true); > >> } > >> > >> +static int link_add(struct rd *rd) > >> +{ > >> + char *name; > >> + char *type = NULL; > >> + char *dev = NULL; > >> + uint32_t seq; > >> + > >> + if (rd_no_arg(rd)) { > >> + pr_err("No link name was supplied\n"); > > I think that it is better to have instruction message and not error > > message: "Please provide ...". > > > Ok.  Perhaps a new utility rd_exec_require_link() can be created? > > > >> + return -EINVAL; > >> + } > >> + name = rd_argv(rd); > >> + rd_arg_inc(rd); > >> + while (!rd_no_arg(rd)) { > >> + if (rd_argv_match(rd, "type")) { > >> + rd_arg_inc(rd); > >> + type = rd_argv(rd); > >> + } else if (rd_argv_match(rd, "netdev")) { > >> + rd_arg_inc(rd); > >> + dev = rd_argv(rd); > >> + } else { > >> + pr_err("Invalid parameter %s\n", rd_argv(rd)); > >> + return -EINVAL; > >> + } > >> + rd_arg_inc(rd); > > Please use chains of struct rd_cmd and rd_exec_cmd() instead of > > open-coding parser. > > > Ok.  Like your recently merged series did... > > > >> + } > >> + if (!type) { > >> + pr_err("No type was supplied\n"); > >> + return -EINVAL; > > General parser handle it. > > > Ok. > > > >> + } > >> + if (!dev) { > >> + pr_err("No net device was supplied\n"); > >> + return -EINVAL; > >> + } > > rd_exec_require_dev() ??? > > > Looks like I can use that.  I'll try it. Actually, in the above code, the variable 'dev' is the netdev string, and that is the 3rd parameter to the 'link add' command, so it isn't appropriate for rd_exec_require_dev(). Steve