| 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(). | expand |
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 >
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 > >
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;
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(-)