On 3/1/23 14:21, Ilya Maximets wrote: > On 3/1/23 14:08, Chris Mi wrote: >> On 3/1/2023 8:44 PM, Ilya Maximets wrote: >>> On 2/28/23 14:05, Chris Mi wrote: >>>> On 2/24/2023 4:16 AM, Ilya Maximets wrote: >>>>> On 2/23/23 12:26, Chris Mi wrote: >>>>>> Initialize psample socket. Add sFlow recv API to receive sampled >>>>>> packets from psample socket. Add sFow recv wait API to add psample >>>>>> socket fd to poll list. >>>>>> >>>>>> Signed-off-by: Chris Mi <[email protected]> >>>>>> Reviewed-by: Roi Dayan <[email protected]> >>>>>> --- >>>>>> lib/dpif.h | 7 ++ >>>>>> lib/netdev-offload-provider.h | 11 ++ >>>>>> lib/netdev-offload-tc.c | 188 ++++++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 206 insertions(+) >>>>>> >>> <snip> >>> >>>>>> +{ >>>>>> + int read_tries = 0; >>>>>> + >>>>>> + if (!psample_sock) { >>>>>> + return ENOENT; >>>>>> + } >>>>>> + >>>>>> + for (;;) { >>>>>> + struct offload_psample psample; >>>>>> + struct offload_sflow sflow; >>>>>> + int error; >>>>>> + >>>>>> + if (++read_tries > 50) { >>>>>> + return EAGAIN; >>>>>> + } >>>>>> + >>>>>> + error = nl_sock_recv(psample_sock, buf, NULL, false); >>>>>> + if (error == ENOBUFS) { >>>>>> + continue; >>>>>> + } >>>>>> + >>>>>> + if (error) { >>>>>> + if (error == EAGAIN) { >>>>>> + break; >>>>>> + } >>>>>> + return error; >>>>>> + } >>>>>> + >>>>>> + error = psample_from_ofpbuf(&psample, buf); >>>>>> + if (!error) { >>>>>> + return psample_parse_packet(&psample, &sflow, upcall); >>>>>> + } else if (error) { >>>>> Condition here is always true. >>>> I copied from dpif_netlink_recv_cpu_dispatch(). And I also think it is ok. >>> The 'if' condition in dpif_netlink_recv_cpu_dispatch() is different >>> and so the 'else if (error)' is not always true there. But it is >>> always true here, hence makes no practical sense. >> OK, I see. I'll change the code like this: >> >> error = psample_from_ofpbuf(&psample, buf); >> if (!error) { >> return psample_parse_packet(&psample, upcall); >> } else { >> return error; >> } > > The 'else' condition is unnecessary, since we do always return > at this point. In the dpif-netlink code there is a case where > we continue, but here there is no such case. > > So, I'd suggest: > > error = psample_from_ofpbuf(&psample, buf); > if (error) { > return error; > } > > return psample_parse_packet(&psample, upcall); > > Or even just a ternary operator: > > error = psample_from_ofpbuf(&psample, buf); > > return error ? error : psample_parse_packet(&psample, upcall);
Just in case, please wait for a proper review on v24 from someone before sending a new version. There is no point to re-spin the set just for this. > > Best regards, Ilya Maximets _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
