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

[Shi Lei]

On 2021-01-07 at 10:52, Shi Lei wrote:
>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].
>>>
>>> -    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;


This piece of code can be simplified as :

    if (resp->nlmsg_type != NLMSG_ERROR &&
        resp->nlmsg_type != NLMSG_DONE) {
            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                                    _("malformed netlink response message"));
            return -1;
    }

When we reach here, virNetlinkTalk have returned 0.
And if (resp->nlmsg_type == NLMSG_ERROR), then (*error) must be 0.

Shi Lei