From: David Ahern <redacted>
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:

quoted

diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c new file mode 100644 index

00000000..942524b7

--- /dev/null
+++ b/vdpa/vdpa.c

@@ -0,0 +1,828 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <stdio.h>
+#include <getopt.h>
+#include <errno.h>
+#include <linux/genetlink.h>
+#include <linux/vdpa.h>
+#include <linux/virtio_ids.h>
+#include <linux/netlink.h>
+#include <libmnl/libmnl.h>
+#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.

quoted

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

quoted

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

quoted

+				   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);

quoted

+
+	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.

quoted

+
+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.

quoted

+
+static int strslashrsplit(char *str, const char **before, const char
+**after) {
+	char *slash;
+
+	slash = strrchr(str, '/');
+	if (!slash)
+		return -EINVAL;
+	*slash = '\0';
+	*before = str;
+	*after = slash + 1;
+	return 0;
+}

similarly here. If you start with things like this in lib/utils you make it easier for
follow on users to find.

Will do.
Thanks for the review.