[ovs-dev] [PATCH 1/2] vconn-stream: Refactor vconn_stream_recv() for readability.

[Ben Pfaff]

On Fri, May 07, 2010 at 06:09:25PM -0700, Jesse Gross wrote:
> On Wed, May 5, 2010 at 10:31 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > Backward "goto" statement are rarely a good idea, so this rewrites that
> > code to be a little more readable.
> >
> 
> This looks correct but to be honest I actually find the old version more
> readable.  When I first looked at this, I thought that the loop could
> potentially execute many times.  It also seems to make more sense that the
> success exit condition should be grouped with the packet length
> determination in the second pass, since that is the only time that it can be
> hit.

You're right.  The new version is at least as confusing.

Here's another try.  Do you like it better?  I think that I do.

(The patch was not easily readable as a unified diff (diff -u), so I
reformatted it as a context diff (diff -c), which is easier to read in
this case.)

--8<--------------------------cut here-------------------------->8--

>From 9366a2bacc59f58d78a424e4e7503490a54c0546 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Mon, 10 May 2010 15:01:25 -0700
Subject: [PATCH] vconn-stream: Refactor vconn_stream_recv() for readability.

Backward "goto" statement are rarely a good idea, so this rewrites that
code for readability.
---
 lib/vconn-stream.c |   86 +++++++++++++++++++++++++++-------------------------
 1 files changed, 45 insertions(+), 41 deletions(-)

diff --git a/lib/vconn-stream.c b/lib/vconn-stream.c
index 034d1d5..0a721d9 100644
*** a/lib/vconn-stream.c
--- b/lib/vconn-stream.c
***************
*** 116,178 ****
  }
  
  static int
! vconn_stream_recv(struct vconn *vconn, struct ofpbuf **bufferp)
  {
!     struct vconn_stream *s = vconn_stream_cast(vconn);
!     struct ofpbuf *rx;
!     size_t want_bytes;
!     ssize_t retval;
  
!     if (s->rxbuf == NULL) {
!         s->rxbuf = ofpbuf_new(1564);
!     }
!     rx = s->rxbuf;
! 
! again:
!     if (sizeof(struct ofp_header) > rx->size) {
!         want_bytes = sizeof(struct ofp_header) - rx->size;
!     } else {
!         struct ofp_header *oh = rx->data;
!         size_t length = ntohs(oh->length);
!         if (length < sizeof(struct ofp_header)) {
!             VLOG_ERR_RL(&rl, "received too-short ofp_header (%zu bytes)",
!                         length);
!             return EPROTO;
!         }
!         want_bytes = length - rx->size;
!         if (!want_bytes) {
!             *bufferp = rx;
!             s->rxbuf = NULL;
!             return 0;
!         }
!     }
      ofpbuf_prealloc_tailroom(rx, want_bytes);
- 
      retval = stream_recv(s->stream, ofpbuf_tail(rx), want_bytes);
      if (retval > 0) {
          rx->size += retval;
!         if (retval == want_bytes) {
!             if (rx->size > sizeof(struct ofp_header)) {
!                 *bufferp = rx;
!                 s->rxbuf = NULL;
!                 return 0;
!             } else {
!                 goto again;
!             }
!         }
!         return EAGAIN;
      } else if (retval == 0) {
          if (rx->size) {
              VLOG_ERR_RL(&rl, "connection dropped mid-packet");
              return EPROTO;
-         } else {
-             return EOF;
          }
      } else {
          return -retval;
      }
  }
  
  static void
  vconn_stream_clear_txbuf(struct vconn_stream *s)
  {
- - 
--- 116,182 ----
  }
  
  static int
! vconn_stream_recv__(struct vconn_stream *s, int rx_len)
  {
!     struct ofpbuf *rx = s->rxbuf;
!     int want_bytes, retval;
  
!     want_bytes = rx_len - rx->size;
      ofpbuf_prealloc_tailroom(rx, want_bytes);
      retval = stream_recv(s->stream, ofpbuf_tail(rx), want_bytes);
      if (retval > 0) {
          rx->size += retval;
!         return retval == want_bytes ? 0 : EAGAIN;
      } else if (retval == 0) {
          if (rx->size) {
              VLOG_ERR_RL(&rl, "connection dropped mid-packet");
              return EPROTO;
          }
+         return EOF;
      } else {
          return -retval;
      }
  }
  
+ static int
+ vconn_stream_recv(struct vconn *vconn, struct ofpbuf **bufferp)
+ {
+     struct vconn_stream *s = vconn_stream_cast(vconn);
+     const struct ofp_header *oh;
+     int rx_len;
+ 
+     /* Allocate new receive buffer if we don't have one. */
+     if (s->rxbuf == NULL) {
+         s->rxbuf = ofpbuf_new(1564);
+     }
+ 
+     /* Read ofp_header. */
+     if (s->rxbuf->size < sizeof(struct ofp_header)) {
+         int retval = vconn_stream_recv__(s, sizeof(struct ofp_header));
+         if (retval) {
+             return retval;
+         }
+     }
+ 
+     /* Read payload. */
+     oh = s->rxbuf->data;
+     rx_len = ntohs(oh->length);
+     if (rx_len < sizeof(struct ofp_header)) {
+         VLOG_ERR_RL(&rl, "received too-short ofp_header (%zu bytes)",
+                     rx_len);
+         return EPROTO;
+     } else if (s->rxbuf->size < rx_len) {
+         int retval = vconn_stream_recv__(s, rx_len);
+         if (retval) {
+             return retval;
+         }
+     }
+ 
+     *bufferp = s->rxbuf;
+     s->rxbuf = NULL;
+     return 0;
+ }
+ 
  static void
  vconn_stream_clear_txbuf(struct vconn_stream *s)
  {
1.6.6.1