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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux