On 3/1/2023 9:23 PM, Ilya Maximets wrote:
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<c...@nvidia.com>
Reviewed-by: Roi Dayan<r...@nvidia.com>
---
  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.
OK.

Eelco,

You reviewed my previous versions and acked most of them. But since Ilya suggested a new design, the code was changed greatly. So I removed the Acked-by for you. I'm wondering
if you have time to review again recently?

Thanks,
Chris

Best regards, Ilya Maximets
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to