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
