On 11/28/2018 12:26 PM, Leon Romanovsky wrote:
> On Thu, Sep 13, 2018 at 10:19:21AM -0700, 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 dev eth0
>> rdma link delete rxe_eth0
>>
>> Signed-off-by: Steve Wise <sw...@opengridcomputing.com>
>> ---
>> rdma/link.c | 106
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> rdma/rdma.h | 1 +
>> rdma/utils.c | 2 +-
>> 3 files changed, 108 insertions(+), 1 deletion(-)
>>
>> diff --git a/rdma/link.c b/rdma/link.c
>> index 7a6d4b7e356d..d4f76b0ce11f 100644
>> --- a/rdma/link.c
>> +++ b/rdma/link.c
>> @@ -14,6 +14,8 @@
>> 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 dev DEV\n", rd->filename);
> I suggest to rename "dev" to be "netdev", because we are using "dev" for
> ib devices.
Yea ok.
>> + pr_out("Usage: %s link delete NAME type TYPE\n", rd->filename);
> Why do you need "type" for "delete" command?
Because the type is used in the kernel to find the appropriate link
ops. I could change the kernel side to search all types for the device
name to delete?
>> return 0;
>> }
>>
>> @@ -315,10 +317,114 @@ static int link_show(struct rd *rd)
>> return rd_exec_link(rd, link_one_show, true);
>> }
>>
>> +static int link_add_parse_cb(const struct nlmsghdr *nlh, void *data)
>> +{
>> + return MNL_CB_OK;
>> +}
>> +
>> +static int link_add(struct rd *rd)
>> +{
>> + char *name;
>> + char *type = NULL;
>> + char *dev = NULL;
>> + uint32_t seq;
>> + int ret;
>> +
>> + if (rd_no_arg(rd)) {
>> + pr_err("No link name was supplied\n");
>> + 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, "dev")) {
>> + rd_arg_inc(rd);
>> + dev = rd_argv(rd);
>> + } else {
>> + pr_err("Invalid parameter %s\n", rd_argv(rd));
>> + return -EINVAL;
>> + }
>> + rd_arg_inc(rd);
>> + }
>> + if (!type) {
>> + pr_err("No type was supplied\n");
>> + return -EINVAL;
>> + }
>> + if (!dev) {
>> + pr_err("No net device was supplied\n");
>> + return -EINVAL;
>> + }
>> +
>> + rd_prepare_msg(rd, RDMA_NLDEV_CMD_NEWLINK, &seq,
>> + (NLM_F_REQUEST | NLM_F_ACK));
>> + mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
>> + mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
>> + mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_NDEV_NAME, dev);
>> + ret = rd_send_msg(rd);
>> + if (ret)
>> + return ret;
>> +
>> + ret = rd_recv_msg(rd, link_add_parse_cb, rd, seq);
>> + if (ret)
>> + perror(NULL);
> Why do you need rd_recv_msg()? I think that it is not needed, at least
> for rename, I didn't need it.
> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/dev.c#n244
To get the response of if it was successfully added. It provides the
errno value.
>> + return ret;
>> +}
>> +
>> +static int link_del_parse_cb(const struct nlmsghdr *nlh, void *data)
>> +{
>> + return MNL_CB_OK;
>> +}
>> +
>> +static int link_del(struct rd *rd)
>> +{
>> + char *name;
>> + char *type = NULL;
>> + uint32_t seq;
>> + int ret;
>> +
>> + if (rd_no_arg(rd)) {
>> + pr_err("No link type was supplied\n");
>> + 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 {
>> + pr_err("Invalid parameter %s\n", rd_argv(rd));
>> + return -EINVAL;
>> + }
>> + rd_arg_inc(rd);
>> + }
>> + if (!type) {
>> + pr_err("No type was supplied\n");
>> + return -EINVAL;
>> + }
>> + rd_prepare_msg(rd, RDMA_NLDEV_CMD_DELLINK, &seq,
>> + (NLM_F_REQUEST | NLM_F_ACK));
>> + mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME, name);
>> + mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_LINK_TYPE, type);
>> + ret = rd_send_msg(rd);
>> + if (ret)
>> + return ret;
>> +
>> + ret = rd_recv_msg(rd, link_del_parse_cb, rd, seq);
>> + if (ret)
>> + perror(NULL);
>> + return ret;
> The same question as above.
>
>> +}
>> +
>> int cmd_link(struct rd *rd)
>> {
>> const struct rd_cmd cmds[] = {
>> { NULL, link_show },
>> + { "add", link_add },
>> + { "delete", link_del },
>> { "show", link_show },
>> { "list", link_show },
>> { "help", link_help },
>> diff --git a/rdma/rdma.h b/rdma/rdma.h
>> index 547bb5749a39..b5271e423c10 100644
>> --- a/rdma/rdma.h
>> +++ b/rdma/rdma.h
>> @@ -79,6 +79,7 @@ struct rd_cmd {
>> */
>> bool rd_no_arg(struct rd *rd);
>> void rd_arg_inc(struct rd *rd);
>> +bool rd_argv_match(struct rd *rd, const char *pattern);
>>
>> char *rd_argv(struct rd *rd);
>>
>> diff --git a/rdma/utils.c b/rdma/utils.c
>> index 61f4aeb1bcf2..41f8f7391279 100644
>> --- a/rdma/utils.c
>> +++ b/rdma/utils.c
>> @@ -32,7 +32,7 @@ int strcmpx(const char *str1, const char *str2)
>> return strncmp(str1, str2, strlen(str1));
>> }
>>
>> -static bool rd_argv_match(struct rd *rd, const char *pattern)
>> +bool rd_argv_match(struct rd *rd, const char *pattern)
>> {
>> if (!rd_argc(rd))
>> return false;
>> --
>> 1.8.3.1
>>
