[PATCH 4/4] netlink: Introduce a helper function to simplify netlink functions
Michal Privoznik
mprivozn at redhat.com
Tue Jan 5 16:00:54 UTC 2021
On 12/21/20 4:23 AM, Shi Lei wrote:
> Extract common code as helper function virNetlinkTalk, then simplify
> the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor].
>
> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
> ---
> src/util/virnetlink.c | 210 +++++++++++++++---------------------------
> src/util/virnetlink.h | 4 +-
> 2 files changed, 78 insertions(+), 136 deletions(-)
Nice cleanup. Patches 1-3 look good.
>
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index fdcb0dc0..7bea38c0 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -353,6 +353,48 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
> return 0;
> }
>
> +
> +static int
> +virNetlinkTalk(const char *ifname,
> + virNetlinkMsg *nl_msg,
> + uint32_t src_pid,
> + uint32_t dst_pid,
> + struct nlmsghdr **resp,
> + unsigned int *resp_len,
> + int *error,
> + virNetlinkTalkFallback fallback)
> +{
> + if (virNetlinkCommand(nl_msg, resp, resp_len,
> + src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
> + return -1;
> +
> + if (*resp_len < NLMSG_LENGTH(0) || resp == NULL)
> + goto malformed_resp;
> +
> + if ((*resp)->nlmsg_type == NLMSG_ERROR) {
> + struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp);
> + if ((*resp)->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> + goto malformed_resp;
> +
> + if (-err->error == EOPNOTSUPP && fallback)
> + return fallback(ifname);
> +
> + if (err->error < 0) {
> + if (error)
> + *error = err->error;
I wonder whether we should report an error here. I mean, if err->error
is set (and not EOPNOTSUPP) then it's stored into *error, good. But -1
is returned ...
> + return -1;
.. here and it's indistinguishable to the caller from -1 returned in
'malformed_resp'. Looking at the usage in next hunks, how about:
if (error)
*error = err->error;
else
virReportSystemError(-err->error, ...);
return -1;
> + }
> + }
> +
> + return 0;
> +
> + malformed_resp:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + return -1;
> +}
> +
> +
> int
> virNetlinkDumpCommand(struct nl_msg *nl_msg,
> virNetlinkDumpCallback callback,
> @@ -396,6 +438,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
> return 0;
> }
>
> +
> /**
> * virNetlinkDumpLink:
> *
> @@ -420,15 +463,13 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
> void **nlData, struct nlattr **tb,
> uint32_t src_pid, uint32_t dst_pid)
> {
> - int rc = -1;
> - struct nlmsgerr *err;
> struct ifinfomsg ifinfo = {
> .ifi_family = AF_UNSPEC,
> .ifi_index = ifindex
> };
> - unsigned int recvbuflen;
> g_autoptr(virNetlinkMsg) nl_msg = NULL;
> g_autofree struct nlmsghdr *resp = NULL;
> + unsigned int resp_len = 0;
>
> if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0)
> return -1;
> @@ -459,46 +500,19 @@ virNetlinkDumpLink(const char *ifname, int ifindex,
> }
> # endif
>
> - if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
> - src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
> + if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid,
> + &resp, &resp_len, NULL, NULL) < 0)
> return -1;
>
> - if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
> - goto malformed_resp;
> -
> - switch (resp->nlmsg_type) {
> - case NLMSG_ERROR:
> - err = (struct nlmsgerr *)NLMSG_DATA(resp);
> - if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> - goto malformed_resp;
> -
> - if (err->error) {
> - virReportSystemError(-err->error,
> - _("error dumping %s (%d) interface"),
> - ifname, ifindex);
Here we'd report an error.
> - return -1;
> - }
> - break;
> -
> - case GENL_ID_CTRL:
> - case NLMSG_DONE:
> - rc = nlmsg_parse(resp, sizeof(struct ifinfomsg),
> - tb, IFLA_MAX, NULL);
> - if (rc < 0)
> - goto malformed_resp;
> - break;
> -
> - default:
> - goto malformed_resp;
> + if ((resp->nlmsg_type != GENL_ID_CTRL && resp->nlmsg_type != NLMSG_DONE) ||
> + nlmsg_parse(resp, sizeof(struct ifinfomsg), tb, IFLA_MAX, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + return -1;
> }
>
> *nlData = g_steal_pointer(&resp);
> return 0;
> -
> - malformed_resp:
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("malformed netlink response message"));
> - return rc;
> }
>
>
> @@ -523,13 +537,12 @@ virNetlinkNewLink(const char *ifname,
> virNetlinkNewLinkDataPtr extra_args,
> int *error)
> {
> - struct nlmsgerr *err;
> struct nlattr *linkinfo = NULL;
> struct nlattr *infodata = NULL;
> - unsigned int buflen;
> struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> g_autoptr(virNetlinkMsg) nl_msg = NULL;
> g_autofree struct nlmsghdr *resp = NULL;
> + unsigned int resp_len = 0;
>
> *error = 0;
>
> @@ -591,37 +604,17 @@ virNetlinkNewLink(const char *ifname,
> }
> }
>
> - if (virNetlinkCommand(nl_msg, &resp, &buflen, 0, 0, NETLINK_ROUTE, 0) < 0)
> + if (virNetlinkTalk(ifname, nl_msg, 0, 0,
> + &resp, &resp_len, error, NULL) < 0)
> return -1;
>
> - if (buflen < NLMSG_LENGTH(0) || resp == NULL)
> - goto malformed_resp;
> -
> - switch (resp->nlmsg_type) {
> - case NLMSG_ERROR:
> - err = (struct nlmsgerr *)NLMSG_DATA(resp);
> - if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> - goto malformed_resp;
> -
> - if (err->error < 0) {
> - *error = err->error;
> - return -1;
But here we wouldn't.
> - }
> - break;
> -
> - case NLMSG_DONE:
> - break;
> -
> - default:
> - goto malformed_resp;
> + if (resp->nlmsg_type != NLMSG_DONE) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + return -1;
> }
>
> return 0;
> -
> - malformed_resp:
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("malformed netlink response message"));
> - return -1;
> }
>
>
> @@ -641,13 +634,12 @@ virNetlinkNewLink(const char *ifname,
> * Returns 0 on success, -1 on fatal error.
> */
> int
> -virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
> +virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback)
> {
> - struct nlmsgerr *err;
> struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> - unsigned int recvbuflen;
> g_autoptr(virNetlinkMsg) nl_msg = NULL;
> g_autofree struct nlmsghdr *resp = NULL;
> + unsigned int resp_len = 0;
>
> nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST);
> if (!nl_msg) {
> @@ -659,44 +651,17 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
>
> NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname);
>
> - if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> - NETLINK_ROUTE, 0) < 0) {
> + if (virNetlinkTalk(ifname, nl_msg, 0, 0,
> + &resp, &resp_len, NULL, fallback) < 0)
> return -1;
> - }
>
> - if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
> - goto malformed_resp;
> -
> - switch (resp->nlmsg_type) {
> - case NLMSG_ERROR:
> - err = (struct nlmsgerr *)NLMSG_DATA(resp);
> - if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> - goto malformed_resp;
> -
> - if (-err->error == EOPNOTSUPP && fallback)
> - return fallback(ifname);
> -
> - if (err->error) {
> - virReportSystemError(-err->error,
> - _("error destroying network device %s"),
> - ifname);
And here we would report an error again.
> - return -1;
> - }
> - break;
> -
> - case NLMSG_DONE:
> - break;
> -
> - default:
> - goto malformed_resp;
> + if (resp->nlmsg_type != NLMSG_DONE) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + return -1;
> }
>
> return 0;
> -
> - malformed_resp:
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("malformed netlink response message"));
> - return -1;
> }
>
> /**
> @@ -712,18 +677,17 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
> *
> * Get neighbor table entry from netlink.
> *
> - * Returns 0 on success, -1 on fatal error.
> + * Returns length of the raw data from netlink on success, -1 on fatal error.
> */
> int
> virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
> {
> - struct nlmsgerr *err;
> struct ndmsg ndinfo = {
> .ndm_family = AF_UNSPEC,
> };
> - unsigned int recvbuflen;
> g_autoptr(virNetlinkMsg) nl_msg = NULL;
> g_autofree struct nlmsghdr *resp = NULL;
> + unsigned int resp_len = 0;
>
> nl_msg = nlmsg_alloc_simple(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST);
> if (!nl_msg) {
> @@ -733,40 +697,18 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid)
>
> NETLINK_MSG_APPEND(nl_msg, sizeof(ndinfo), &ndinfo);
>
> - if (virNetlinkCommand(nl_msg, &resp, &recvbuflen,
> - src_pid, dst_pid, NETLINK_ROUTE, 0) < 0)
> + if (virNetlinkTalk(NULL, nl_msg, src_pid, dst_pid,
> + &resp, &resp_len, NULL, NULL) < 0)
> return -1;
>
> - if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
> - goto malformed_resp;
> -
> - switch (resp->nlmsg_type) {
> - case NLMSG_ERROR:
> - err = (struct nlmsgerr *)NLMSG_DATA(resp);
> - if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> - goto malformed_resp;
> -
> - if (err->error) {
> - virReportSystemError(-err->error,
> - "%s", _("error dumping"));
> - return -1;
Ditto.
> - }
> - break;
> -
> - case RTM_NEWNEIGH:
> - break;
> -
> - default:
> - goto malformed_resp;
> + if (resp->nlmsg_type != RTM_NEWNEIGH) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + return -1;
> }
>
> *nlData = g_steal_pointer(&resp);
> - return recvbuflen;
> -
> - malformed_resp:
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("malformed netlink response message"));
> - return -1;
> + return resp_len;
> }
>
Michal
More information about the libvir-list
mailing list