Re: [PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions

On 1/11/21 3:23 AM, Shi Lei wrote: 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) */ So this sets @error to a negative number. [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. */ 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