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@xxxxxxxxxxxxxx>
---
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
