RE: [PATCH iproute2-next 2/2] vdpa: Add vdpa tool
> From: David Ahern > Sent: Tuesday, January 26, 2021 9:53 AM > > Looks fine. A few comments below around code re-use. > > On 1/22/21 4:26 AM, Parav Pandit wrote: > > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c new file mode 100644 index > > ..942524b7 > > --- /dev/null > > +++ b/vdpa/vdpa.c > > @@ -0,0 +1,828 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include "mnl_utils.h" > > + > > +#include "version.h" > > +#include "json_print.h" > > +#include "utils.h" > > + > > +static int g_indent_level; > > + > > +#define INDENT_STR_STEP 2 > > +#define INDENT_STR_MAXLEN 32 > > +static char g_indent_str[INDENT_STR_MAXLEN + 1] = ""; > > The indent code has a lot of parallels with devlink -- including helpers below > around indent_inc and _dec. Please take a look at how to refactor and re- > use. > Ok. Devlink has some more convoluted code with next line etc. But I will see if I can consolidate without changing the devlink's flow/logic. > > + > > +struct vdpa_socket { > > + struct mnl_socket *nl; > > + char *buf; > > + uint32_t family; > > + unsigned int seq; > > +}; > > + > > +static int vdpa_socket_sndrcv(struct vdpa_socket *nlg, const struct > nlmsghdr *nlh, > > + mnl_cb_t data_cb, void *data) { > > + int err; > > + > > + err = mnl_socket_sendto(nlg->nl, nlh, nlh->nlmsg_len); > > + if (err < 0) { > > + perror("Failed to send data"); > > + return -errno; > > + } > > + > > + err = mnlu_socket_recv_run(nlg->nl, nlh->nlmsg_seq, nlg->buf, > MNL_SOCKET_BUFFER_SIZE, > > + data_cb, data); > > + if (err < 0) { > > + fprintf(stderr, "vdpa answers: %s\n", strerror(errno)); > > + return -errno; > > + } > > + return 0; > > +} > > + > > +static int get_family_id_attr_cb(const struct nlattr *attr, void > > +*data) { > > + int type = mnl_attr_get_type(attr); > > + const struct nlattr **tb = data; > > + > > + if (mnl_attr_type_valid(attr, CTRL_ATTR_MAX) < 0) > > + return MNL_CB_ERROR; > > + > > + if (type == CTRL_ATTR_FAMILY_ID && > > + mnl_attr_validate(attr, MNL_TYPE_U16) < 0) > > + return MNL_CB_ERROR; > > + tb[type] = attr; > > + return MNL_CB_OK; > > +} > > + > > +static int get_family_id_cb(const struct nlmsghdr *nlh, void *data) { > > + struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh); > > + struct nlattr *tb[CTRL_ATTR_MAX + 1] = {}; > > + uint32_t *p_id = data; > > + > > + mnl_attr_parse(nlh, sizeof(*genl), get_family_id_attr_cb, tb); > > + if (!tb[CTRL_ATTR_FAMILY_ID]) > > + return MNL_CB_ERROR; > > + *p_id = mnl_attr_get_u16(tb[CTRL_ATTR_FAMILY_ID]); > > + return MNL_CB_OK; > > +} > > + > > +static int family_get(struct vdpa_socket *nlg) { > > + struct genlmsghdr hdr = {}; > > + struct nlmsghdr *nlh; > > + int err; > > + > > + hdr.cmd = CTRL_CMD_GETFAMILY; > > + hdr.version = 0x1; > > + > > + nlh = mnlu_msg_prepare(nlg->buf, GENL_ID_CTRL, > > + NLM_F_REQUEST | NLM_F_ACK, > > + &hdr, sizeof(hdr)); > > + > > + mnl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, > VDPA_GENL_NAME); > > + > > + err = mnl_socket_sendto(nlg->nl, nlh, nlh->nlmsg_len); > > + if (err < 0) > > + return err; > > + > > + err = mnlu_socket_recv_run(nlg->nl, nlh->nlmsg_seq, nlg->buf, > > + MNL_SOCKET_BUFFER_SIZE, > > + get_family_id_cb, &nlg->family); > > + return err; > > +} > > + > > +static int vdpa_socket_open(struct vdpa_socket *nlg) { > > + int err; > > + > > + nlg->buf = malloc(MNL_SOCKET_BUFFER_SIZE); > > + if (!nlg->buf) > > + goto err_buf_alloc; > > + > > + nlg->nl = mnlu_socket_open(NETLINK_GENERIC); > > + if (!nlg->nl) > > + goto err_socket_open; > > + > > + err = family_get(nlg); > > + if (err) > > + goto err_socket; > > + > > + return 0; > > + > > +err_socket: > > + mnl_socket_close(nlg->nl); > > +err_socket_open: > > + free(nlg->buf); > > +err_buf_alloc: > > + return -1; > > +} > > The above 4 functions duplicate a lot of devlink functionality. Please create > a > helper in lib/mnl_utils.c that can be used in both. > Will do. > > + > > +static unsigned int strslashcount(char *str) { > > + unsigned int count = 0; > > + char *pos = str; > > + > > + while ((pos = strchr(pos, '/'))) { > > + count++; > > + pos++; > > + } > > + return count; > > +} > > you could make that a generic function (e.g., str_char_count) by passing '/' > as > an input. > Yes. > > + > > +static int strslashrsplit(char *str, const char **before, const char > > +**after) { > > + char *slash; > > + > > + slash = strrchr(str, '/'); > > + if (!slash) > > + return -EINVAL; > > + *slash = '\0'; > > + *before = str;
Re: [PATCH iproute2-next 2/2] vdpa: Add vdpa tool
Looks fine. A few comments below around code re-use. On 1/22/21 4:26 AM, Parav Pandit wrote: > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c > new file mode 100644 > index ..942524b7 > --- /dev/null > +++ b/vdpa/vdpa.c > @@ -0,0 +1,828 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "mnl_utils.h" > + > +#include "version.h" > +#include "json_print.h" > +#include "utils.h" > + > +static int g_indent_level; > + > +#define INDENT_STR_STEP 2 > +#define INDENT_STR_MAXLEN 32 > +static char g_indent_str[INDENT_STR_MAXLEN + 1] = ""; The indent code has a lot of parallels with devlink -- including helpers below around indent_inc and _dec. Please take a look at how to refactor and re-use. > + > +struct vdpa_socket { > + struct mnl_socket *nl; > + char *buf; > + uint32_t family; > + unsigned int seq; > +}; > + > +static int vdpa_socket_sndrcv(struct vdpa_socket *nlg, const struct nlmsghdr > *nlh, > + mnl_cb_t data_cb, void *data) > +{ > + int err; > + > + err = mnl_socket_sendto(nlg->nl, nlh, nlh->nlmsg_len); > + if (err < 0) { > + perror("Failed to send data"); > + return -errno; > + } > + > + err = mnlu_socket_recv_run(nlg->nl, nlh->nlmsg_seq, nlg->buf, > MNL_SOCKET_BUFFER_SIZE, > +data_cb, data); > + if (err < 0) { > + fprintf(stderr, "vdpa answers: %s\n", strerror(errno)); > + return -errno; > + } > + return 0; > +} > + > +static int get_family_id_attr_cb(const struct nlattr *attr, void *data) > +{ > + int type = mnl_attr_get_type(attr); > + const struct nlattr **tb = data; > + > + if (mnl_attr_type_valid(attr, CTRL_ATTR_MAX) < 0) > + return MNL_CB_ERROR; > + > + if (type == CTRL_ATTR_FAMILY_ID && > + mnl_attr_validate(attr, MNL_TYPE_U16) < 0) > + return MNL_CB_ERROR; > + tb[type] = attr; > + return MNL_CB_OK; > +} > + > +static int get_family_id_cb(const struct nlmsghdr *nlh, void *data) > +{ > + struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh); > + struct nlattr *tb[CTRL_ATTR_MAX + 1] = {}; > + uint32_t *p_id = data; > + > + mnl_attr_parse(nlh, sizeof(*genl), get_family_id_attr_cb, tb); > + if (!tb[CTRL_ATTR_FAMILY_ID]) > + return MNL_CB_ERROR; > + *p_id = mnl_attr_get_u16(tb[CTRL_ATTR_FAMILY_ID]); > + return MNL_CB_OK; > +} > + > +static int family_get(struct vdpa_socket *nlg) > +{ > + struct genlmsghdr hdr = {}; > + struct nlmsghdr *nlh; > + int err; > + > + hdr.cmd = CTRL_CMD_GETFAMILY; > + hdr.version = 0x1; > + > + nlh = mnlu_msg_prepare(nlg->buf, GENL_ID_CTRL, > +NLM_F_REQUEST | NLM_F_ACK, > +&hdr, sizeof(hdr)); > + > + mnl_attr_put_strz(nlh, CTRL_ATTR_FAMILY_NAME, VDPA_GENL_NAME); > + > + err = mnl_socket_sendto(nlg->nl, nlh, nlh->nlmsg_len); > + if (err < 0) > + return err; > + > + err = mnlu_socket_recv_run(nlg->nl, nlh->nlmsg_seq, nlg->buf, > +MNL_SOCKET_BUFFER_SIZE, > +get_family_id_cb, &nlg->family); > + return err; > +} > + > +static int vdpa_socket_open(struct vdpa_socket *nlg) > +{ > + int err; > + > + nlg->buf = malloc(MNL_SOCKET_BUFFER_SIZE); > + if (!nlg->buf) > + goto err_buf_alloc; > + > + nlg->nl = mnlu_socket_open(NETLINK_GENERIC); > + if (!nlg->nl) > + goto err_socket_open; > + > + err = family_get(nlg); > + if (err) > + goto err_socket; > + > + return 0; > + > +err_socket: > + mnl_socket_close(nlg->nl); > +err_socket_open: > + free(nlg->buf); > +err_buf_alloc: > + return -1; > +} The above 4 functions duplicate a lot of devlink functionality. Please create a helper in lib/mnl_utils.c that can be used in both. > + > +static void vdpa_socket_close(struct vdpa_socket *nlg) > +{ > + mnl_socket_close(nlg->nl); > + free(nlg->buf); > +} > + > +#define VDPA_OPT_MGMTDEV_HANDLE BIT(0) > +#define VDPA_OPT_VDEV_MGMTDEV_HANDLE BIT(1) > +#define VDPA_OPT_VDEV_NAME BIT(2) > +#define VDPA_OPT_VDEV_HANDLE BIT(3) > + > +struct vdpa_opts { > + uint64_t present; /* flags of present items */ > + const char *mdev_bus_name; > + const char *mdev_name; > + const char *vdev_name; > + unsigned int device_id; > +}; > + > +struct vdpa { > + struct vdpa_socket nlg; > + struct vdpa_opts opts; > + bool json_output; > +}; > + > +static void indent_inc(void) > +{ > + if (g_indent_level + INDENT_STR_STEP > INDENT_STR_MAXLEN) > + return; > + g_indent_level += INDENT_STR_STEP; > + memset(g_indent_str, ' ', sizeof(g_indent_str)); > + g_indent_str[g_indent_level] = '\0'; > +} > + > +static void inden
[PATCH iproute2-next 2/2] vdpa: Add vdpa tool
vdpa tool is created to create, delete and query vdpa devices. examples: Show vdpa management device that supports creating, deleting vdpa devices. $ vdpa mgmtdev show vdpasim: supported_classes net $ vdpa mgmtdev show -jp { "show": { "vdpasim": { "supported_classes": [ "net" ] } } } Create a vdpa device of type networking named as "foo2" from the management device vdpasim_net: $ vdpa dev add mgmtdev vdpasim_net name foo2 Show the newly created vdpa device by its name: $ vdpa dev show foo2 foo2: type network mgmtdev vdpasim_net vendor_id 0 max_vqs 2 max_vq_size 256 $ vdpa dev show foo2 -jp { "dev": { "foo2": { "type": "network", "mgmtdev": "vdpasim_net", "vendor_id": 0, "max_vqs": 2, "max_vq_size": 256 } } } Delete the vdpa device after its use: $ vdpa dev del foo2 Signed-off-by: Parav Pandit --- Makefile| 2 +- man/man8/vdpa-dev.8 | 96 + man/man8/vdpa-mgmtdev.8 | 53 +++ man/man8/vdpa.8 | 76 vdpa/Makefile | 24 ++ vdpa/vdpa.c | 828 6 files changed, 1078 insertions(+), 1 deletion(-) create mode 100644 man/man8/vdpa-dev.8 create mode 100644 man/man8/vdpa-mgmtdev.8 create mode 100644 man/man8/vdpa.8 create mode 100644 vdpa/Makefile create mode 100644 vdpa/vdpa.c diff --git a/Makefile b/Makefile index e64c6599..19bd163e 100644 --- a/Makefile +++ b/Makefile @@ -55,7 +55,7 @@ WFLAGS += -Wmissing-declarations -Wold-style-definition -Wformat=2 CFLAGS := $(WFLAGS) $(CCOPTS) -I../include -I../include/uapi $(DEFINES) $(CFLAGS) YACCFLAGS = -d -t -v -SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man +SUBDIRS=lib ip tc bridge misc netem genl tipc devlink rdma dcb man vdpa LIBNETLINK=../lib/libutil.a ../lib/libnetlink.a LDLIBS += $(LIBNETLINK) diff --git a/man/man8/vdpa-dev.8 b/man/man8/vdpa-dev.8 new file mode 100644 index ..36433519 --- /dev/null +++ b/man/man8/vdpa-dev.8 @@ -0,0 +1,96 @@ +.TH DEVLINK\-DEV 8 "5 Jan 2021" "iproute2" "Linux" +.SH NAME +vdpa-dev \- vdpa device configuration +.SH SYNOPSIS +.sp +.ad l +.in +8 +.ti -8 +.B vdpa +.B dev +.RI "[ " OPTIONS " ] " +.RI " { " COMMAND | " " +.BR help " }" +.sp + +.ti -8 +.IR OPTIONS " := { " +\fB\-V\fR[\fIersion\fR] +} + +.ti -8 +.B vdpa dev show +.RI "[ " DEV " ]" + +.ti -8 +.B vdpa dev help + +.ti -8 +.B vdpa dev add +.B name +.I NAME +.B mgmtdev +.I MGMTDEV + +.ti -8 +.B vdpa dev del +.I DEV + +.SH "DESCRIPTION" +.SS vdpa dev show - display vdpa device attributes + +.PP +.I "DEV" +- specifies the vdpa device to show. +If this argument is omitted all devices are listed. + +.in +4 +Format is: +.in +2 +VDPA_DEVICE_NAME + +.SS vdpa dev add - add a new vdpa device. + +.TP +.BI name " NAME" +Name of the new vdpa device to add. + +.TP +.BI mgmtdev " MGMTDEV" +Name of the management device to use for device addition. + +.SS vdpa dev del - Delete the vdpa device. + +.PP +.I "DEV" +- specifies the vdpa device to delete. + +.SH "EXAMPLES" +.PP +vdpa dev show +.RS 4 +Shows the all vdpa devices on the system. +.RE +.PP +vdpa dev show foo +.RS 4 +Shows the specified vdpa device. +.RE +.PP +vdpa dev add name foo mgmtdev vdpa_sim_net +.RS 4 +Add the vdpa device named foo on the management device vdpa_sim_net. +.RE +.PP +vdpa dev del foo +.RS 4 +Delete the vdpa device named foo which was previously created. +.RE + +.SH SEE ALSO +.BR vdpa (8), +.BR vdpa-mgmtdev (8), +.br + +.SH AUTHOR +Parav Pandit diff --git a/man/man8/vdpa-mgmtdev.8 b/man/man8/vdpa-mgmtdev.8 new file mode 100644 index ..cae2cbd0 --- /dev/null +++ b/man/man8/vdpa-mgmtdev.8 @@ -0,0 +1,53 @@ +.TH DEVLINK\-DEV 8 "5 Jan 2021" "iproute2" "Linux" +.SH NAME +vdpa-dev \- vdpa management device view +.SH SYNOPSIS +.sp +.ad l +.in +8 +.ti -8 +.B vdpa +.B mgmtdev +.RI " { " COMMAND | " " +.BR help " }" +.sp + +.ti -8 +.IR OPTIONS " := { " +\fB\-V\fR[\fIersion\fR] +} + +.ti -8 +.B vdpa mgmtdev show +.RI "[ " MGMTDEV " ]" + +.ti -8 +.B vdpa mgmtdev help + +.SH "DESCRIPTION" +.SS vdpa mgmtdev show - display vdpa management device attributes + +.PP +.I "MGMTDEV" +- specifies the vdpa management device to show. +If this argument is omitted all management devices are listed. + +.SH "EXAMPLES" +.PP +vdpa mgmtdev show +.RS 4 +Shows all the vdpa management devices on the system. +.RE +.PP +vdpa mgmtdev show bar +.RS 4 +Shows the specified vdpa management device. +.RE + +.SH SEE ALSO +.BR vdpa (8), +.BR vdpa-dev (8), +.br + +.SH AUTHOR +Parav Pandit diff --git a/man/man8/vdpa.8 b/man/man8/vdpa.8 new file mode 100644 index ..d1aaecec --- /dev/null +++ b/man/man8/vdpa.8 @@ -0,0 +1,76 @@ +.TH VDPA 8 "5 Jan 2021" "iproute2" "Linux" +.SH NAME +vdpa \- vdpa management tool +.SH SYNOPSIS +.sp +.ad l +.in +8 +.ti -8 +.B vdpa +.RI "[ " OPTIONS " ] { " dev | mgmtdev " } { " COMMAND " | " +.BR help " }" +.sp + +.SH OPTIONS + +.TP +.BR "\-V" , " --