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,
> > + , 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, >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,
> +, 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, >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
[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" , "
