On 1/11/21 3: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@massclouds.com> --- src/util/virnetlink.c | 232 ++++++++++++++++++------------------------ src/util/virnetlink.h | 4 +- 2 files changed, 101 insertions(+), 135 deletions(-)
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index fdcb0dc0..650acff7 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -353,6 +353,54 @@ int virNetlinkCommand(struct nl_msg *nl_msg, return 0; }
+
I guess we should document that NLMSG_ERROR and err->error == 0 is a valid case and no error. How about: /** * virNetlinkTalk: * @ifname: name of the link * @nl_msg: pointer to netlink message * @src_pid: pid used for nl_pid of the local end of the netlink message * (0 == "use getpid()") * @dst_pid: pid of destination nl_pid if the kernel * is not the target of the netlink message but it is to be * sent to another process (0 if sending to the kernel) * @resp: pointer to pointer where response buffer will be allocated * @resp_len: pointer to integer holding the size of the response buffer * on return of the function * @error: pointer to store netlink error (-errno) * @fallback: pointer to an alternate function that will be called in case * netlink fails with EOPNOTSUPP (any other error will simply be * treated as an error) * * Simple wrapper around virNetlinkCommand(). The returned netlink message * is allocated at @resp. Please note that according to netlink(7) man page, * reply with type of NLMSG_ERROR and @error == 0 is an acknowledgment and * thus not an error. * * Returns: 0 on success, * -1 otherwise (error reported if @error == NULL) */
+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; + + 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;
So this sets @error to a negative number. [1]
+ else + virReportSystemError(-err->error, "%s", _("netlink error")); + + return -1; + }
And here I'd put: /* According to netlink(7) man page NLMSG_ERROR packet with error * field set to 0 is an acknowledgment packet and thus not an error. */
+ } + + 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 +444,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg, return 0; }
+ /** * virNetlinkDumpLink: * @@ -420,15 +469,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 +507,25 @@ 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,
1: and here it is used as if it was positive. We need -error. And in the rest of places too. Anyway, I can fix that before pushing, so that you don't have to send another version. Do you agree if this is squashed in? diff --git c/src/util/virnetlink.c w/src/util/virnetlink.c index 650acff7d7..9bd7339054 100644 --- c/src/util/virnetlink.c +++ w/src/util/virnetlink.c @@ -354,6 +354,31 @@ int virNetlinkCommand(struct nl_msg *nl_msg, } +/** + * virNetlinkTalk: + * @ifname: name of the link + * @nl_msg: pointer to netlink message + * @src_pid: pid used for nl_pid of the local end of the netlink message + * (0 == "use getpid()") + * @dst_pid: pid of destination nl_pid if the kernel + * is not the target of the netlink message but it is to be + * sent to another process (0 if sending to the kernel) + * @resp: pointer to pointer where response buffer will be allocated + * @resp_len: pointer to integer holding the size of the response buffer + * on return of the function + * @error: pointer to store netlink error (-errno) + * @fallback: pointer to an alternate function that will be called in case + * netlink fails with EOPNOTSUPP (any other error will simply be + * treated as an error) + * + * Simple wrapper around virNetlinkCommand(). The returned netlink message + * is allocated at @resp. Please note that according to netlink(7) man page, + * reply with type of NLMSG_ERROR and @error == 0 is an acknowledgment and + * thus not an error. + * + * Returns: 0 on success, + * -1 otherwise (error reported if @error == NULL) + */ static int virNetlinkTalk(const char *ifname, virNetlinkMsg *nl_msg, @@ -390,6 +415,9 @@ virNetlinkTalk(const char *ifname, return -1; } + + /* According to netlink(7) man page NLMSG_ERROR packet with error + * field set to 0 is an acknowledgment packet and thus not an error. */ } return 0; @@ -509,7 +537,7 @@ virNetlinkDumpLink(const char *ifname, int ifindex, if (virNetlinkTalk(ifname, nl_msg, src_pid, dst_pid, &resp, &resp_len, &error, NULL) < 0) { - virReportSystemError(error, + virReportSystemError(-error, _("error dumping %s (%d) interface"), ifname, ifindex); return -1; @@ -668,7 +696,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback) if (virNetlinkTalk(ifname, nl_msg, 0, 0, &resp, &resp_len, &error, fallback) < 0) { - virReportSystemError(error, + virReportSystemError(-error, _("error destroying network device %s"), ifname); return -1; @@ -720,7 +748,7 @@ virNetlinkGetNeighbor(void **nlData, uint32_t src_pid, uint32_t dst_pid) if (virNetlinkTalk(NULL, nl_msg, src_pid, dst_pid, &resp, &resp_len, &error, NULL) < 0) { - virReportSystemError(error, "%s", _("error dumping")); + virReportSystemError(-error, "%s", _("error dumping neighbor table")); return -1; } Michal