On 2021-01-06 at 18:21, Michal Privoznik wrote:
>On 1/6/21 3:40 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 | 225 +++++++++++++++++-------------------------
>> src/util/virnetlink.h | 4 +-
>> 2 files changed, 94 insertions(+), 135 deletions(-)
>>
>> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
>> index fdcb0dc0..2936a3ef 100644
>> --- a/src/util/virnetlink.c
>> +++ b/src/util/virnetlink.c
>> @@ -353,6 +353,52 @@ 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)
>
>This needs to be *resp == NULL, because now we're passing a pointer to a
>pointer.
Uh. It's my bad. I'll fix it.
>
>> + goto malformed_resp;
>> +
>> + if ((*resp)->nlmsg_type == NLMSG_ERROR) {
>> + struct nlmsgerr *err = (struct nlmsgerr *) NLMSG_DATA(resp);
>
>And this needs to be NLMSG_DATA(*resp);
Yes!
>Also, might be worth putting an empty line here to create two blocks:
>one with variable declaration and the other with the code.
Okay.
>
>
>> + 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"));
>
>Since this is a two line body, it should be wrapped in curly braces
>(according to our coding style) which means that both bodies should have
>them (we don't really like one body having them while the other not).
I feel like it can be :
if (error)
*error = err->error;
else
virReportSystemError(-err->error, "%s", _("netlink error"));
Since ^this line isn't more than 80 columns.
>> +
>> + 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 +442,7 @@ virNetlinkDumpCommand(struct nl_msg *nl_msg,
>> return 0;
>> }
>>
>> +
>> /**
>> * virNetlinkDumpLink:
>> *
>> @@ -420,15 +467,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 +505,23 @@ 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 != 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 +546,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 +613,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;
>> - }
>> - 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;
>> }
>
>This is not correct IMO. The way that control flows through this
>function before your patch is (I'm trying to create virbr0 using 'virsh
>net-start default'):
>
>1) virNetlinkCommand() suceeds,
>2) resp->nlmsg_type == NLMSG_ERROR even though the bridge was created,
>3) err->error is 0 hence we hit 'break' and fall out of the switch and ..
>
>>
>> return 0;
Yes.
So the result ((resp->nlmsg_type == NLMSG_ERROR) && (err->error == 0)) should
be regarded as a success.
I feel like that this piece of code can be like:
if (((resp->nlmsg_type != NLMSG_ERROR) || (*error != 0)) &&
(resp->nlmsg_type != NLMSG_DONE) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("malformed netlink response message"));
return -1;
}
return 0;
And the other three functions (DumpLink||DelLink|GetNeighbor) have the same
problem, I will fix them also.
>
>.. return 0. With your patch, I get the "malrofmed netlink response
>message" error. Reading netlink(7) manpage lets more light into this:
>
> For reliable transfer the sender can request an acknowledgement from
> the receiver by setting the NLM_F_ACK flag. An acknowledgment is an
> NLMSG_ERROR packet with the error field set to 0.
>
>Which is exactly what I'm seeing, but what I don't understand is why we
>are getting this ack message since NLM_F_ACK flag was not set.
It seems that the auto-ack is the default behaviour for netlink internal socket.
The netlink socke will be with NLM_F_ACK unless we call nl_socket_disable_auto_ack
explicitely.
Shi Lei
>
>Michal
>
