8:23 p.m.
V2 here: https://www.redhat.com/archives/libvir-list/2021-January/msg00174.html
Since V2:
- Fix the post process of virNetlinkTalk().
If virNetlinkTalk() succeeds and we got an NLMSG_ERROR packet with
the zero-value error, it should be regarded as a success.
- Several fixes for the pointer of resp in virNetlinkTalk().
- Minor fixes according to coding style.
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 | 321 +++++++++++++++++++-----------------------
src/util/virnetlink.h | 27 +---
2 files changed, 149 insertions(+), 199 deletions(-)
--
2.25.1
8:23 p.m.
New subject: [PATCHv3 1/4] netlink: Remove invalid flags(NLM_F_CREATE and NLM_F_EXCL) for RTM_DELLINK
NLM_F_CREATE and NLM_F_EXCL are invalid for RTM_DELLINK,
so remove them.
Signed-off-by: Shi Lei <shi_lei(a)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
8:23 p.m.
New subject: [PATCHv3 2/4] netlink: Minor changes for macros NETLINK_MSG_[NEST_START|NEST_END|PUT]
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(a)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
8:23 p.m.
New subject: [PATCHv3 3/4] netlink: Introduce macro NETLINK_MSG_APPEND to wrap nlmsg_append
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(a)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
8:23 p.m.
New subject: [PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions
Extract common code as helper function virNetlinkTalk, then simplify
the functions virNetlink[DumpLink|NewLink|DelLink|GetNeighbor].
Signed-off-by: Shi Lei <shi_lei(a)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;
}
+
+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;
+ 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 +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,
+ _("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 != NLMSG_ERROR &&
+ 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 +550,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 +617,18 @@ 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_ERROR &&
+ 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 +648,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 +666,22 @@ 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_ERROR &&
+ 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 +697,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 +718,21 @@ 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 != NLMSG_ERROR &&
+ 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 +1266,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
5:26 a.m.
New subject: [PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions
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(a)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
7:29 p.m.
New subject: [PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions
On 2021-01-11 at 19:26, Michal Privoznik wrote:
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(a)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
I agree. Thanks!
Shi Lei
5:51 a.m.
New subject: [PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions
On 1/12/21 2:29 AM, Shi Lei wrote:
On 2021-01-11 at 19:26, Michal Privoznik wrote:
> 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(a)massclouds.com>
>> ---
>> src/util/virnetlink.c | 232 ++++++++++++++++++------------------------
>> src/util/virnetlink.h | 4 +-
>> 2 files changed, 101 insertions(+), 135 deletions(-)
>>
>
> 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?
>
I agree. Thanks!
Okay, we are currently in feature freeze preparing for release. I will
push it afterwards.
Michal
4:46 a.m.
New subject: [PATCHv3 4/4] netlink: Introduce a helper function to simplify netlink functions
On 1/13/21 12:51 PM, Michal Privoznik wrote:
On 1/12/21 2:29 AM, Shi Lei wrote:
> On 2021-01-11 at 19:26, Michal Privoznik wrote:
>> 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(a)massclouds.com>
>>> ---
>>> src/util/virnetlink.c | 232
>>> ++++++++++++++++++------------------------
>>> src/util/virnetlink.h | 4 +-
>>> 2 files changed, 101 insertions(+), 135 deletions(-)
>>>
>>
>> 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?
>>
>
> I agree. Thanks!
>
Okay, we are currently in feature freeze preparing for release. I will
push it afterwards.
Pushed now.
Michal
1443
days inactive
1447
days old
8 comments
2 participants
participants (2)
-
Michal Privoznik -
Shi Lei