From: Parav Pandit <parav@nvidia.com>
To: David Ahern <dsahern@gmail.com>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"mst@redhat.com" <mst@redhat.com>,
"jasowang@redhat.com" <jasowang@redhat.com>
Subject: RE: [PATCH iproute2-next 2/2] vdpa: Add vdpa tool
Date: Tue, 26 Jan 2021 13:26:09 +0000 [thread overview]
Message-ID: <BY5PR12MB4322CD2FF36716821AB3EB9CDCBC9@BY5PR12MB4322.namprd12.prod.outlook.com> (raw)
In-Reply-To: <dc59454f-5e8b-2fce-9837-44808df933d4@gmail.com>
> From: David Ahern <dsahern@gmail.com>
> 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
> > 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.
> > +
> > +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;
> > + *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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-01-26 13:26 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-22 11:26 [PATCH iproute2-next 0/2] Add vdpa device management tool Parav Pandit
2021-01-22 11:26 ` [PATCH iproute2-next 1/2] Add kernel headers Parav Pandit
2021-01-22 11:26 ` [PATCH iproute2-next 2/2] vdpa: Add vdpa tool Parav Pandit
2021-01-26 4:22 ` David Ahern
2021-01-26 13:26 ` Parav Pandit [this message]
2021-01-28 18:43 ` [PATCH iproute2-next v2 0/2] Add vdpa device management tool Parav Pandit
2021-01-28 18:43 ` [PATCH iproute2-next v2 1/2] Add kernel headers Parav Pandit
2021-01-28 18:43 ` [PATCH iproute2-next v2 2/2] vdpa: Add vdpa tool Parav Pandit
2021-01-29 3:23 ` Parav Pandit
2021-02-02 10:35 ` [PATCH iproute2-next v3 0/5] Add vdpa device management tool Parav Pandit
2021-02-02 10:35 ` [PATCH iproute2-next v3 1/5] Add kernel headers Parav Pandit
2021-02-04 1:37 ` David Ahern
2021-02-05 17:54 ` Parav Pandit
2021-02-02 10:35 ` [PATCH iproute2-next v3 2/5] utils: Add helper routines for indent handling Parav Pandit
2021-02-02 10:35 ` [PATCH iproute2-next v3 3/5] utils: Add generic socket helpers Parav Pandit
2021-02-02 10:35 ` [PATCH iproute2-next v3 4/5] utils: Add helper to map string to unsigned int Parav Pandit
2021-02-02 10:35 ` [PATCH iproute2-next v3 5/5] vdpa: Add vdpa tool Parav Pandit
2021-02-04 3:16 ` [PATCH iproute2-next v3 0/5] Add vdpa device management tool Jason Wang
2021-02-04 11:15 ` Adrian Moreno
2021-02-05 3:40 ` Jason Wang
2021-02-05 17:53 ` Parav Pandit
2021-02-05 18:06 ` Adrian Moreno
2021-02-05 18:07 ` Adrian Moreno
2021-02-05 18:10 ` [PATCH iproute2-next v4 " Parav Pandit
2021-02-05 18:10 ` [PATCH iproute2-next v4 1/5] Add kernel headers Parav Pandit
2021-02-08 15:47 ` David Ahern
2021-02-10 18:28 ` Parav Pandit
2021-02-05 18:10 ` [PATCH iproute2-next v4 2/5] utils: Add helper routines for indent handling Parav Pandit
2021-02-05 18:10 ` [PATCH iproute2-next v4 3/5] utils: Add generic socket helpers Parav Pandit
2021-02-05 18:10 ` [PATCH iproute2-next v4 4/5] utils: Add helper to map string to unsigned int Parav Pandit
2021-02-05 18:10 ` [PATCH iproute2-next v4 5/5] vdpa: Add vdpa tool Parav Pandit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BY5PR12MB4322CD2FF36716821AB3EB9CDCBC9@BY5PR12MB4322.namprd12.prod.outlook.com \
--to=parav@nvidia.com \
--cc=dsahern@gmail.com \
--cc=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).