Yes, some cases where the transaction is completed with an error we need to increase the "done" counter, This case would be different than the case where the transaction didn't go through. Eitan
-----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Alin Serdean Sent: Tuesday, April 14, 2015 2:19 PM To: Ben Pfaff; Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] netlink-socket: Exit NL transaction loop when EINVAL is returned In nl_sock_transact_multiple__ we do the following: if (!DeviceIoControl(sock->handle, OVS_IOCTL_TRANSACT, txn->request->data, txn->request->size, reply_buf, sizeof reply_buf, &reply_len, NULL)) { /* XXX: Map to a more appropriate error. */ error = EINVAL; break; } We map every failure to EINVAL that is why it did not pop out into Linux. We should definitely log the error using ovs_lasterror_to_string before setting it to EINVAL. Maybe we should just increase the number of transactions in some situations(i.e. STATUS_INVALID_PARAMETER) as an idea to allow the rest of the transactions to be processed. Alin. -----Mesaj original----- De la: dev [mailto:dev-boun...@openvswitch.org] În numele Ben Pfaff Trimis: Tuesday, April 14, 2015 11:42 PM Către: Sorin Vinturis Cc: dev@openvswitch.org Subiect: Re: [ovs-dev] [PATCH] netlink-socket: Exit NL transaction loop when EINVAL is returned On Tue, Apr 14, 2015 at 08:25:59PM +0000, Sorin Vinturis wrote: > The nl_sock_transact_multiple function enters in an infinite loop, > when invalid error, EINVAL, is returned by nl_sock_transact_multiple__. > EINVAL is the error returned by the latter function when a driver > request fails. > > Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> > Reported-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com> > Reported-at: > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs-2Dissues_issues_57&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=YVmZuxZuPYrnWO888OUuBR52fT8FJiccWxGc1MINlzo&s=TyVETpKzpDFZ0fNq_BkMdUZ2ZWIiKOCnNjGFbftWyp0&e= > I see that this fixes a bug, even on Linux. Thank you. It's actually a pretty serious bug (given the infinite loop), but I guess that it must not occur in any normal circumstances, otherwise we would have heard about it over the years. However, I want to make sure of something before I commit it. nl_sock_transact_multiple__() should only return an error in the case of a "transport" error, that is, of some problem communicating with the datapath (e.g. the kernel module has been removed or something similarly fatal). It should not return an error in cases where some message asks the datapath to do something erroneous (e.g. to add a flow that the datapath doesn't understand, to delete a vport that doesn't exist, ...). This is because only in the former case should all of the transactions be aborted; in the latter case, any remaining transactions should still be processed. _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=YVmZuxZuPYrnWO888OUuBR52fT8FJiccWxGc1MINlzo&s=UmlNVfETCHyYzoXJ3xE5k5-JXC_h_V1gLAT1hKo6bbk&e= _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=CWsgHUxi6ExLXY798tmo3LJ4e3geGYp56lkcH-5cLCY&m=YVmZuxZuPYrnWO888OUuBR52fT8FJiccWxGc1MINlzo&s=UmlNVfETCHyYzoXJ3xE5k5-JXC_h_V1gLAT1hKo6bbk&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
