[PATCHv2 0/4] netlink: Extract common code to simplify netlink functions
V1 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00836.html Since V1: - Minor fixes for reporting system error in Patch 4. Shi Lei (4): netlink: Remove invalid flags(NLM_F_CREATE and NLM_F_EXCL) for RTM_DELLINK netlink: Minor changes for macros NETLINK_MSG_[NEST_START|NEST_END|PUT] netlink: Introduce macro NETLINK_MSG_APPEND to wrap nlmsg_append netlink: Introduce a helper function to simplify netlink functions src/util/virnetlink.c | 314 +++++++++++++++++++----------------------- src/util/virnetlink.h | 27 +--- 2 files changed, 142 insertions(+), 199 deletions(-) -- 2.25.1
NLM_F_CREATE and NLM_F_EXCL are invalid for RTM_DELLINK, so remove them. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index ca735bb8..17e6eeb9 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -627,8 +627,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) g_autoptr(virNetlinkMsg) nl_msg = NULL; g_autofree struct nlmsghdr *resp = NULL; - nl_msg = nlmsg_alloc_simple(RTM_DELLINK, - NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST); if (!nl_msg) { virReportOOMError(); return -1; -- 2.25.1
Move macros NETLINK_MSG_[NEST_START|NEST_END|PUT] from .h into .c; within these macros, replace 'goto' with reporting error and returning; simplify virNetlinkDumpLink and virNetlinkDelLink by using NETLINK_MSG_PUT. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 44 +++++++++++++++++++++++++++++++++---------- src/util/virnetlink.h | 23 ---------------------- 2 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 17e6eeb9..0b55b124 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -44,6 +44,35 @@ VIR_LOG_INIT("util.netlink"); # include <linux/veth.h> +# define NETLINK_MSG_NEST_START(msg, container, attrtype) \ +do { \ + container = nla_nest_start(msg, attrtype); \ + if (!container) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("allocated netlink buffer is too small")); \ + return -1; \ + } \ +} while (0) + +# define NETLINK_MSG_NEST_END(msg, container) \ +do { nla_nest_end(msg, container); } while (0) + +/* + * we need to use an intermediary pointer to @data as compilers may sometimes + * complain about @data not being a pointer type: + * error: the address of 'foo' will always evaluate as 'true' [-Werror=address] + */ +# define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ +do { \ + const void *dataptr = data; \ + if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("allocated netlink buffer is too small")); \ + return -1; \ + } \ +} while (0) + + /* State for a single netlink event handle */ struct virNetlinkEventHandle { int watch; @@ -406,10 +435,8 @@ virNetlinkDumpLink(const char *ifname, int ifindex, if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) goto buffer_too_small; - if (ifname) { - if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; - } + if (ifname) + NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); # ifdef RTEXT_FILTER_VF /* if this filter exists in the kernel's netlink implementation, @@ -419,10 +446,8 @@ virNetlinkDumpLink(const char *ifname, int ifindex, { uint32_t ifla_ext_mask = RTEXT_FILTER_VF; - if (nla_put(nl_msg, IFLA_EXT_MASK, - sizeof(ifla_ext_mask), &ifla_ext_mask) < 0) { - goto buffer_too_small; - } + NETLINK_MSG_PUT(nl_msg, IFLA_EXT_MASK, + sizeof(ifla_ext_mask), &ifla_ext_mask); } # endif @@ -636,8 +661,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) goto buffer_too_small; - if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; + NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) { diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 7c4ed202..966d6db3 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -38,29 +38,6 @@ struct nlmsghdr; #endif /* WITH_LIBNL */ -#define NETLINK_MSG_NEST_START(msg, container, attrtype) \ -do { \ - container = nla_nest_start(msg, attrtype); \ - if (!container) \ - goto buffer_too_small; \ -} while(0) - -#define NETLINK_MSG_NEST_END(msg, container) \ -do { nla_nest_end(msg, container); } while(0) - -/* - * we need to use an intermediary pointer to @data as compilers may sometimes - * complain about @data not being a pointer type: - * error: the address of 'foo' will always evaluate as 'true' [-Werror=address] - */ -#define NETLINK_MSG_PUT(msg, attrtype, datalen, data) \ -do { \ - const void *dataptr = data; \ - if (dataptr && nla_put(msg, attrtype, datalen, dataptr) < 0) \ - goto buffer_too_small; \ -} while(0) - - int virNetlinkStartup(void); void virNetlinkShutdown(void); -- 2.25.1
Introduce a macro NETLINK_MSG_APPEND to wrap nlmsg_append and simplify code. Remove those labels 'buffer_too_small', since they are now useless. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 42 +++++++++++++----------------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0b55b124..fdcb0dc0 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -72,6 +72,15 @@ do { \ } \ } while (0) +# define NETLINK_MSG_APPEND(msg, datalen, dataptr) \ +do { \ + if (nlmsg_append(msg, dataptr, datalen, NLMSG_ALIGNTO) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ + _("allocated netlink buffer is too small")); \ + return -1; \ + } \ +} while (0) + /* State for a single netlink event handle */ struct virNetlinkEventHandle { @@ -432,8 +441,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex, return -1; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + NETLINK_MSG_APPEND(nl_msg, sizeof(ifinfo), &ifinfo); if (ifname) NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); @@ -491,11 +499,6 @@ virNetlinkDumpLink(const char *ifname, int ifindex, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return rc; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return rc; } @@ -545,8 +548,7 @@ virNetlinkNewLink(const char *ifname, return -1; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + NETLINK_MSG_APPEND(nl_msg, sizeof(ifinfo), &ifinfo); NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); @@ -620,11 +622,6 @@ virNetlinkNewLink(const char *ifname, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -1; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return -1; } @@ -658,8 +655,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) return -1; } - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; + NETLINK_MSG_APPEND(nl_msg, sizeof(ifinfo), &ifinfo); NETLINK_MSG_PUT(nl_msg, IFLA_IFNAME, (strlen(ifname) + 1), ifname); @@ -701,11 +697,6 @@ virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -1; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return -1; } /** @@ -740,9 +731,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid) return -1; } - if (nlmsg_append(nl_msg, &ndinfo, sizeof(ndinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; - + NETLINK_MSG_APPEND(nl_msg, sizeof(ndinfo), &ndinfo); if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) @@ -778,11 +767,6 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -1; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - return -1; } int -- 2.25.1
Extract common code as helper function virNetlinkTalk, then simplify the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor]. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetlink.c | 225 +++++++++++++++++------------------------- src/util/virnetlink.h | 4 +- 2 files changed, 94 insertions(+), 135 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index fdcb0dc0..2936a3ef 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -353,6 +353,52 @@ 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; + else + virReportSystemError(-err->error, + "%s", _("netlink 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 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, return 0; } + /** * virNetlinkDumpLink: * @@ -420,15 +467,14 @@ 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; + int error = 0; if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) return -1; @@ -459,46 +505,23 @@ 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, &error, NULL) < 0) { + virReportSystemError(error, + _("error dumping %s (%d) interface"), + ifname, ifindex); 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); - 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 +546,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 +613,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; - } - 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 +643,13 @@ 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; + int error = 0; nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST); if (!nl_msg) { @@ -659,44 +661,21 @@ 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, &error, fallback) < 0) { + virReportSystemError(error, + _("error destroying network device %s"), + ifname); 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); - 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 +691,18 @@ 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; + int error = 0; nl_msg = nlmsg_alloc_simple(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST); if (!nl_msg) { @@ -733,40 +712,20 @@ 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, &error, NULL) < 0) { + virReportSystemError(error, "%s", _("error dumping")); 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; - } - 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; } int @@ -1300,7 +1259,7 @@ virNetlinkDumpLink(const char *ifname G_GNUC_UNUSED, int virNetlinkDelLink(const char *ifname G_GNUC_UNUSED, - virNetlinkDelLinkFallback fallback G_GNUC_UNUSED) + virNetlinkTalkFallback fallback G_GNUC_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 966d6db3..cab685fe 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -69,9 +69,9 @@ int virNetlinkNewLink(const char *ifname, virNetlinkNewLinkDataPtr data, int *error); -typedef int (*virNetlinkDelLinkFallback)(const char *ifname); +typedef int (*virNetlinkTalkFallback)(const char *ifname); -int virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback); +int virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback); int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen); -- 2.25.1
On 1/6/21 3:40 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@massclouds.com> --- src/util/virnetlink.c | 225 +++++++++++++++++------------------------- src/util/virnetlink.h | 4 +- 2 files changed, 94 insertions(+), 135 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index fdcb0dc0..2936a3ef 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -353,6 +353,52 @@ 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)
This needs to be *resp == NULL, because now we're passing a pointer to a pointer.
+ goto malformed_resp; + + if ((*resp)->nlmsg_type == NLMSG_ERROR) { + struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp);
And this needs to be NLMSG_DATA(*resp); Also, might be worth putting an empty line here to create two blocks: one with variable declaration and the other with the code.
+ 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; + else + virReportSystemError(-err->error, + "%s", _("netlink error"));
Since this is a two line body, it should be wrapped in curly braces (according to our coding style) which means that both bodies should have them (we don't really like one body having them while the other not).
+ + 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 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, return 0; }
+ /** * virNetlinkDumpLink: * @@ -420,15 +467,14 @@ 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; + int error = 0;
if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) return -1; @@ -459,46 +505,23 @@ 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, &error, NULL) < 0) { + virReportSystemError(error, + _("error dumping %s (%d) interface"), + ifname, ifindex); 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); - 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 +546,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 +613,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; - } - 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; }
This is not correct IMO. The way that control flows through this function before your patch is (I'm trying to create virbr0 using 'virsh net-start default'): 1) virNetlinkCommand() suceeds, 2) resp->nlmsg_type == NLMSG_ERROR even though the bridge was created, 3) err->error is 0 hence we hit 'break' and fall out of the switch and ..
return 0;
.. return 0. With your patch, I get the "malrofmed netlink response message" error. Reading netlink(7) manpage lets more light into this: For reliable transfer the sender can request an acknowledgement from the receiver by setting the NLM_F_ACK flag. An acknowledgment is an NLMSG_ERROR packet with the error field set to 0. Which is exactly what I'm seeing, but what I don't understand is why we are getting this ack message since NLM_F_ACK flag was not set. Michal
On 2021-01-06 at 18:21, Michal Privoznik wrote:
On 1/6/21 3:40 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@massclouds.com> --- src/util/virnetlink.c | 225 +++++++++++++++++------------------------- src/util/virnetlink.h | 4 +- 2 files changed, 94 insertions(+), 135 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index fdcb0dc0..2936a3ef 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -353,6 +353,52 @@ 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)
This needs to be *resp == NULL, because now we're passing a pointer to a pointer.
Uh. It's my bad. I'll fix it.
+ goto malformed_resp; + + if ((*resp)->nlmsg_type == NLMSG_ERROR) { + struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp);
And this needs to be NLMSG_DATA(*resp);
Yes!
Also, might be worth putting an empty line here to create two blocks: one with variable declaration and the other with the code.
Okay.
+ 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; + else + virReportSystemError(-err->error, + "%s", _("netlink error"));
Since this is a two line body, it should be wrapped in curly braces (according to our coding style) which means that both bodies should have them (we don't really like one body having them while the other not).
I feel like it can be : if (error) *error = err->error; else virReportSystemError(-err->error, "%s", _("netlink error")); Since ^this line isn't more than 80 columns.
+ + 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 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, return 0; } + /** * virNetlinkDumpLink: * @@ -420,15 +467,14 @@ 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; + int error = 0; if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) return -1; @@ -459,46 +505,23 @@ 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, &error, NULL) < 0) { + virReportSystemError(error, + _("error dumping %s (%d) interface"), + ifname, ifindex); 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); - 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 +546,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 +613,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; - } - 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; }
This is not correct IMO. The way that control flows through this function before your patch is (I'm trying to create virbr0 using 'virsh net-start default'):
1) virNetlinkCommand() suceeds, 2) resp->nlmsg_type == NLMSG_ERROR even though the bridge was created, 3) err->error is 0 hence we hit 'break' and fall out of the switch and ..
return 0;
Yes. So the result ((resp->nlmsg_type == NLMSG_ERROR) && (err->error == 0)) should be regarded as a success. I feel like that this piece of code can be like: if (((resp->nlmsg_type != NLMSG_ERROR) || (*error != 0)) && (resp->nlmsg_type != NLMSG_DONE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -1; } return 0; And the other three functions (DumpLink||DelLink|GetNeighbor) have the same problem, I will fix them also.
.. return 0. With your patch, I get the "malrofmed netlink response message" error. Reading netlink(7) manpage lets more light into this:
For reliable transfer the sender can request an acknowledgement from the receiver by setting the NLM_F_ACK flag. An acknowledgment is an NLMSG_ERROR packet with the error field set to 0.
Which is exactly what I'm seeing, but what I don't understand is why we are getting this ack message since NLM_F_ACK flag was not set.
It seems that the auto-ack is the default behaviour for netlink internal socket. The netlink socke will be with NLM_F_ACK unless we call nl_socket_disable_auto_ack explicitely. Shi Lei
Michal
On 2021-01-07 at 10:52, Shi Lei wrote:
On 2021-01-06 at 18:21, Michal Privoznik wrote:
On 1/6/21 3:40 AM, Shi Lei wrote:
Extract common code as helper function virNetlinkTalk, then simplify the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor].
- default: - goto malformed_resp; + if (resp->nlmsg_type != NLMSG_DONE) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed netlink response message")); + return -1; }
This is not correct IMO. The way that control flows through this function before your patch is (I'm trying to create virbr0 using 'virsh net-start default'):
1) virNetlinkCommand() suceeds, 2) resp->nlmsg_type == NLMSG_ERROR even though the bridge was created, 3) err->error is 0 hence we hit 'break' and fall out of the switch and ..
return 0;
Yes. So the result ((resp->nlmsg_type == NLMSG_ERROR) && (err->error == 0)) should be regarded as a success.
I feel like that this piece of code can be like:
if (((resp->nlmsg_type != NLMSG_ERROR) || (*error != 0)) && (resp->nlmsg_type != NLMSG_DONE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -1; }
return 0;
This piece of code can be simplified as : if (resp->nlmsg_type != NLMSG_ERROR && resp->nlmsg_type != NLMSG_DONE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed netlink response message")); return -1; } When we reach here, virNetlinkTalk have returned 0. And if (resp->nlmsg_type == NLMSG_ERROR), then (*error) must be 0. Shi Lei
On 1/5/21 11:40 PM, Shi Lei wrote:
V1 here: https://www.redhat.com/archives/libvir-list/2020-December/msg00836.html
Since V1: - Minor fixes for reporting system error in Patch 4.
Shi Lei (4): netlink: Remove invalid flags(NLM_F_CREATE and NLM_F_EXCL) for RTM_DELLINK netlink: Minor changes for macros NETLINK_MSG_[NEST_START|NEST_END|PUT] netlink: Introduce macro NETLINK_MSG_APPEND to wrap nlmsg_append netlink: Introduce a helper function to simplify netlink functions
Series LGTM Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
src/util/virnetlink.c | 314 +++++++++++++++++++----------------------- src/util/virnetlink.h | 27 +--- 2 files changed, 142 insertions(+), 199 deletions(-)
participants (3)
-
Daniel Henrique Barboza -
Michal Privoznik -
Shi Lei