[ovs-dev] ofp-flow: Fix return value for ofputil_decode_flow_stats_reply().

Message ID 20180213184900.8787-1-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] ofp-flow: Fix return value for ofputil_decode_flow_stats_reply().
Related show

Commit Message

Ben Pfaff Feb. 13, 2018, 6:49 p.m.
This function returned errno values for some errors and OFPERR_* values
for others.  The callers all expected OFPERR_* values.  This fixes the
problem.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ofp-flow.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

Comments

Yifeng Sun Feb. 13, 2018, 11:14 p.m. | #1
Looks good to me. Thanks for the correction.

Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>

On Tue, Feb 13, 2018 at 10:49 AM, Ben Pfaff <blp@ovn.org> wrote:

> This function returned errno values for some errors and OFPERR_* values
> for others.  The callers all expected OFPERR_* values.  This fixes the
> problem.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/ofp-flow.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
> index 8876b7be5eb1..c29f5b7cb05f 100644
> --- a/lib/ofp-flow.c
> +++ b/lib/ofp-flow.c
> @@ -752,7 +752,7 @@ parse_ofp_flow_stats_request_str(struct
> ofputil_flow_stats_request *fsr,
>   * of it.  'fs->ofpacts' will point into the 'ofpacts' buffer.
>   *
>   * Returns 0 if successful, EOF if no replies were left in this 'msg',
> - * otherwise a positive errno value. */
> + * otherwise an OFPERR_* value. */
>  int
>  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
>                                  struct ofpbuf *msg,
> @@ -783,20 +783,21 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
>          if (!ofs) {
>              VLOG_WARN_RL(&rl, "OFPST_FLOW reply has %"PRIu32" leftover "
>                           "bytes at end", msg->size);
> -            return EINVAL;
> +            return OFPERR_OFPBRC_BAD_LEN;
>          }
>
>          length = ntohs(ofs->length);
>          if (length < sizeof *ofs) {
>              VLOG_WARN_RL(&rl, "OFPST_FLOW reply claims invalid "
>                           "length %"PRIuSIZE, length);
> -            return EINVAL;
> +            return OFPERR_OFPBRC_BAD_LEN;
>          }
>
> -        if (ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
> -                                     &padded_match_len)) {
> +        error = ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
> +                                         &padded_match_len);
> +        if (error) {
>              VLOG_WARN_RL(&rl, "OFPST_FLOW reply bad match");
> -            return EINVAL;
> +            return error;
>          }
>          instructions_len = length - sizeof *ofs - padded_match_len;
>
> @@ -833,14 +834,14 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
>          if (!ofs) {
>              VLOG_WARN_RL(&rl, "OFPST_FLOW reply has %"PRIu32" leftover "
>                           "bytes at end", msg->size);
> -            return EINVAL;
> +            return OFPERR_OFPBRC_BAD_LEN;
>          }
>
>          length = ntohs(ofs->length);
>          if (length < sizeof *ofs) {
>              VLOG_WARN_RL(&rl, "OFPST_FLOW reply claims invalid "
>                           "length %"PRIuSIZE, length);
> -            return EINVAL;
> +            return OFPERR_OFPBRC_BAD_LEN;
>          }
>          instructions_len = length - sizeof *ofs;
>
> @@ -866,7 +867,7 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
>          if (!nfs) {
>              VLOG_WARN_RL(&rl, "NXST_FLOW reply has %"PRIu32" leftover "
>                           "bytes at end", msg->size);
> -            return EINVAL;
> +            return OFPERR_OFPBRC_BAD_LEN;
>          }
>
>          length = ntohs(nfs->length);
> @@ -874,11 +875,12 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
>          if (length < sizeof *nfs + ROUND_UP(match_len, 8)) {
>              VLOG_WARN_RL(&rl, "NXST_FLOW reply with match_len=%"PRIuSIZE"
> "
>                           "claims invalid length %"PRIuSIZE, match_len,
> length);
> -            return EINVAL;
> +            return OFPERR_OFPBRC_BAD_LEN;
>          }
> -        if (nx_pull_match(msg, match_len, &fs->match, NULL, NULL, false,
> NULL,
> -                          NULL)) {
> -            return EINVAL;
> +        error = nx_pull_match(msg, match_len, &fs->match, NULL, NULL,
> false,
> +                              NULL, NULL);
> +        if (error) {
> +            return error;
>          }
>          instructions_len = length - sizeof *nfs - ROUND_UP(match_len, 8);
>
> @@ -907,10 +909,12 @@ ofputil_decode_flow_stats_reply(struct
> ofputil_flow_stats *fs,
>          OVS_NOT_REACHED();
>      }
>
> -    if (ofpacts_pull_openflow_instructions(msg, instructions_len,
> oh->version,
> -                                           NULL, NULL, ofpacts)) {
> +    error = ofpacts_pull_openflow_instructions(msg, instructions_len,
> +                                               oh->version, NULL, NULL,
> +                                               ofpacts);
> +    if (error) {
>          VLOG_WARN_RL(&rl, "OFPST_FLOW reply bad instructions");
> -        return EINVAL;
> +        return error;
>      }
>      fs->ofpacts = ofpacts->data;
>      fs->ofpacts_len = ofpacts->size;
> --
> 2.16.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Feb. 14, 2018, 12:24 a.m. | #2
Thanks for the review.  I applied this to master.

On Tue, Feb 13, 2018 at 03:14:56PM -0800, Yifeng Sun wrote:
> Looks good to me. Thanks for the correction.
> 
> Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
> 
> On Tue, Feb 13, 2018 at 10:49 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> > This function returned errno values for some errors and OFPERR_* values
> > for others.  The callers all expected OFPERR_* values.  This fixes the
> > problem.
> >
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  lib/ofp-flow.c | 36 ++++++++++++++++++++----------------
> >  1 file changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
> > index 8876b7be5eb1..c29f5b7cb05f 100644
> > --- a/lib/ofp-flow.c
> > +++ b/lib/ofp-flow.c
> > @@ -752,7 +752,7 @@ parse_ofp_flow_stats_request_str(struct
> > ofputil_flow_stats_request *fsr,
> >   * of it.  'fs->ofpacts' will point into the 'ofpacts' buffer.
> >   *
> >   * Returns 0 if successful, EOF if no replies were left in this 'msg',
> > - * otherwise a positive errno value. */
> > + * otherwise an OFPERR_* value. */
> >  int
> >  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
> >                                  struct ofpbuf *msg,
> > @@ -783,20 +783,21 @@ ofputil_decode_flow_stats_reply(struct
> > ofputil_flow_stats *fs,
> >          if (!ofs) {
> >              VLOG_WARN_RL(&rl, "OFPST_FLOW reply has %"PRIu32" leftover "
> >                           "bytes at end", msg->size);
> > -            return EINVAL;
> > +            return OFPERR_OFPBRC_BAD_LEN;
> >          }
> >
> >          length = ntohs(ofs->length);
> >          if (length < sizeof *ofs) {
> >              VLOG_WARN_RL(&rl, "OFPST_FLOW reply claims invalid "
> >                           "length %"PRIuSIZE, length);
> > -            return EINVAL;
> > +            return OFPERR_OFPBRC_BAD_LEN;
> >          }
> >
> > -        if (ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
> > -                                     &padded_match_len)) {
> > +        error = ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
> > +                                         &padded_match_len);
> > +        if (error) {
> >              VLOG_WARN_RL(&rl, "OFPST_FLOW reply bad match");
> > -            return EINVAL;
> > +            return error;
> >          }
> >          instructions_len = length - sizeof *ofs - padded_match_len;
> >
> > @@ -833,14 +834,14 @@ ofputil_decode_flow_stats_reply(struct
> > ofputil_flow_stats *fs,
> >          if (!ofs) {
> >              VLOG_WARN_RL(&rl, "OFPST_FLOW reply has %"PRIu32" leftover "
> >                           "bytes at end", msg->size);
> > -            return EINVAL;
> > +            return OFPERR_OFPBRC_BAD_LEN;
> >          }
> >
> >          length = ntohs(ofs->length);
> >          if (length < sizeof *ofs) {
> >              VLOG_WARN_RL(&rl, "OFPST_FLOW reply claims invalid "
> >                           "length %"PRIuSIZE, length);
> > -            return EINVAL;
> > +            return OFPERR_OFPBRC_BAD_LEN;
> >          }
> >          instructions_len = length - sizeof *ofs;
> >
> > @@ -866,7 +867,7 @@ ofputil_decode_flow_stats_reply(struct
> > ofputil_flow_stats *fs,
> >          if (!nfs) {
> >              VLOG_WARN_RL(&rl, "NXST_FLOW reply has %"PRIu32" leftover "
> >                           "bytes at end", msg->size);
> > -            return EINVAL;
> > +            return OFPERR_OFPBRC_BAD_LEN;
> >          }
> >
> >          length = ntohs(nfs->length);
> > @@ -874,11 +875,12 @@ ofputil_decode_flow_stats_reply(struct
> > ofputil_flow_stats *fs,
> >          if (length < sizeof *nfs + ROUND_UP(match_len, 8)) {
> >              VLOG_WARN_RL(&rl, "NXST_FLOW reply with match_len=%"PRIuSIZE"
> > "
> >                           "claims invalid length %"PRIuSIZE, match_len,
> > length);
> > -            return EINVAL;
> > +            return OFPERR_OFPBRC_BAD_LEN;
> >          }
> > -        if (nx_pull_match(msg, match_len, &fs->match, NULL, NULL, false,
> > NULL,
> > -                          NULL)) {
> > -            return EINVAL;
> > +        error = nx_pull_match(msg, match_len, &fs->match, NULL, NULL,
> > false,
> > +                              NULL, NULL);
> > +        if (error) {
> > +            return error;
> >          }
> >          instructions_len = length - sizeof *nfs - ROUND_UP(match_len, 8);
> >
> > @@ -907,10 +909,12 @@ ofputil_decode_flow_stats_reply(struct
> > ofputil_flow_stats *fs,
> >          OVS_NOT_REACHED();
> >      }
> >
> > -    if (ofpacts_pull_openflow_instructions(msg, instructions_len,
> > oh->version,
> > -                                           NULL, NULL, ofpacts)) {
> > +    error = ofpacts_pull_openflow_instructions(msg, instructions_len,
> > +                                               oh->version, NULL, NULL,
> > +                                               ofpacts);
> > +    if (error) {
> >          VLOG_WARN_RL(&rl, "OFPST_FLOW reply bad instructions");
> > -        return EINVAL;
> > +        return error;
> >      }
> >      fs->ofpacts = ofpacts->data;
> >      fs->ofpacts_len = ofpacts->size;
> > --
> > 2.16.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

Patch

diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c
index 8876b7be5eb1..c29f5b7cb05f 100644
--- a/lib/ofp-flow.c
+++ b/lib/ofp-flow.c
@@ -752,7 +752,7 @@  parse_ofp_flow_stats_request_str(struct ofputil_flow_stats_request *fsr,
  * of it.  'fs->ofpacts' will point into the 'ofpacts' buffer.
  *
  * Returns 0 if successful, EOF if no replies were left in this 'msg',
- * otherwise a positive errno value. */
+ * otherwise an OFPERR_* value. */
 int
 ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
                                 struct ofpbuf *msg,
@@ -783,20 +783,21 @@  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         if (!ofs) {
             VLOG_WARN_RL(&rl, "OFPST_FLOW reply has %"PRIu32" leftover "
                          "bytes at end", msg->size);
-            return EINVAL;
+            return OFPERR_OFPBRC_BAD_LEN;
         }
 
         length = ntohs(ofs->length);
         if (length < sizeof *ofs) {
             VLOG_WARN_RL(&rl, "OFPST_FLOW reply claims invalid "
                          "length %"PRIuSIZE, length);
-            return EINVAL;
+            return OFPERR_OFPBRC_BAD_LEN;
         }
 
-        if (ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
-                                     &padded_match_len)) {
+        error = ofputil_pull_ofp11_match(msg, NULL, NULL, &fs->match,
+                                         &padded_match_len);
+        if (error) {
             VLOG_WARN_RL(&rl, "OFPST_FLOW reply bad match");
-            return EINVAL;
+            return error;
         }
         instructions_len = length - sizeof *ofs - padded_match_len;
 
@@ -833,14 +834,14 @@  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         if (!ofs) {
             VLOG_WARN_RL(&rl, "OFPST_FLOW reply has %"PRIu32" leftover "
                          "bytes at end", msg->size);
-            return EINVAL;
+            return OFPERR_OFPBRC_BAD_LEN;
         }
 
         length = ntohs(ofs->length);
         if (length < sizeof *ofs) {
             VLOG_WARN_RL(&rl, "OFPST_FLOW reply claims invalid "
                          "length %"PRIuSIZE, length);
-            return EINVAL;
+            return OFPERR_OFPBRC_BAD_LEN;
         }
         instructions_len = length - sizeof *ofs;
 
@@ -866,7 +867,7 @@  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         if (!nfs) {
             VLOG_WARN_RL(&rl, "NXST_FLOW reply has %"PRIu32" leftover "
                          "bytes at end", msg->size);
-            return EINVAL;
+            return OFPERR_OFPBRC_BAD_LEN;
         }
 
         length = ntohs(nfs->length);
@@ -874,11 +875,12 @@  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         if (length < sizeof *nfs + ROUND_UP(match_len, 8)) {
             VLOG_WARN_RL(&rl, "NXST_FLOW reply with match_len=%"PRIuSIZE" "
                          "claims invalid length %"PRIuSIZE, match_len, length);
-            return EINVAL;
+            return OFPERR_OFPBRC_BAD_LEN;
         }
-        if (nx_pull_match(msg, match_len, &fs->match, NULL, NULL, false, NULL,
-                          NULL)) {
-            return EINVAL;
+        error = nx_pull_match(msg, match_len, &fs->match, NULL, NULL, false,
+                              NULL, NULL);
+        if (error) {
+            return error;
         }
         instructions_len = length - sizeof *nfs - ROUND_UP(match_len, 8);
 
@@ -907,10 +909,12 @@  ofputil_decode_flow_stats_reply(struct ofputil_flow_stats *fs,
         OVS_NOT_REACHED();
     }
 
-    if (ofpacts_pull_openflow_instructions(msg, instructions_len, oh->version,
-                                           NULL, NULL, ofpacts)) {
+    error = ofpacts_pull_openflow_instructions(msg, instructions_len,
+                                               oh->version, NULL, NULL,
+                                               ofpacts);
+    if (error) {
         VLOG_WARN_RL(&rl, "OFPST_FLOW reply bad instructions");
-        return EINVAL;
+        return error;
     }
     fs->ofpacts = ofpacts->data;
     fs->ofpacts_len = ofpacts->size;