On 2021-01-06 at 00:00, Michal Privoznik wrote:
>On 12/21/20 4: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 | 210 +++++++++++++++---------------------------
>> src/util/virnetlink.h | 4 +-
>> 2 files changed, 78 insertions(+), 136 deletions(-)
>
>Nice cleanup. Patches 1-3 look good.
Okay.
>
>>
>> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
>> index fdcb0dc0..7bea38c0 100644
>> --- a/src/util/virnetlink.c
>> +++ b/src/util/virnetlink.c
>> @@ -353,6 +353,48 @@ 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;
>
>I wonder whether we should report an error here. I mean, if err->error
>is set (and not EOPNOTSUPP) then it's stored into *error, good. But -1
>is returned ...
>
>> + return -1;
>
>.. here and it's indistinguishable to the caller from -1 returned in
>'malformed_resp'. Looking at the usage in next hunks, how about:
>
> if (error)
> *error = err->error;
> else
> virReportSystemError(-err->error, ...);
>
> return -1;
>
Okay.
>> + }
>> + }
>> +
>> + 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 +438,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
>> return 0;
>> }
>>
>> +
>> /**
>> * virNetlinkDumpLink:
>> *
>> @@ -420,15 +463,13 @@ 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;
>>
>> if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0)
>> return -1;
>> @@ -459,46 +500,19 @@ 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, NULL, NULL) < 0)
>> 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);
>
>
>Here we'd report an error.
Okay.
>
>> - 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 +537,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 +604,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;
>
>But here we wouldn't.
Okay.
>
>> - }
>> - 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 +634,12 @@ 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;
>>
>> nl_msg = nlmsg_alloc_simple(RTM_DELLINK, NLM_F_REQUEST);
>> if (!nl_msg) {
>> @@ -659,44 +651,17 @@ 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, NULL, fallback) < 0)
>> 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);
>
>
>And here we would report an error again.
Okay.
>
>> - 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 +677,17 @@ 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;
>>
>> nl_msg = nlmsg_alloc_simple(RTM_GETNEIGH, NLM_F_DUMP | NLM_F_REQUEST);
>> if (!nl_msg) {
>> @@ -733,40 +697,18 @@ 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, NULL, NULL) < 0)
>> 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;
>
>Ditto.
Okay.
>
>> - }
>> - 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;
>> }
>>
>
>Michal
>
Thanks!
Shi Lei
