| Message ID | 1447200827-31045-1-git-send-email-blp@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Nov 10, 2015 at 04:13:47PM -0800, Ben Pfaff wrote: > The _error version should be used to report errors. > > Also, add missing return in one error case. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > v1->v2: Add missing return in error case (thanks Cascardo!). > > lib/ovs-router.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/lib/ovs-router.c b/lib/ovs-router.c > index 2f093e8..619aa3a 100644 > --- a/lib/ovs-router.c > +++ b/lib/ovs-router.c > @@ -275,7 +275,8 @@ ovs_router_add(struct unixctl_conn *conn, int argc, > gw6 = in6addr_any; > } > } else { > - unixctl_command_reply(conn, "Invalid parameters"); > + unixctl_command_reply_error(conn, "Invalid parameters"); > + return; This is OK, included in the first patch. > } > ovs_router_insert__(plen + 32, &ip6, plen, argv[2], &gw6); > unixctl_command_reply(conn, "OK"); OK. > @@ -293,13 +294,13 @@ ovs_router_del(struct unixctl_conn *conn, int argc OVS_UNUSED, > in6_addr_set_mapped_ipv4(&ip6, ip); > plen += 96; > } else if (!scan_ipv6_route(argv[1], &ip6, &plen)) { > - unixctl_command_reply(conn, "Invalid parameters"); > + unixctl_command_reply_error(conn, "Invalid parameters"); > } My bad. This should have that return too. That amounts to three missing returns. I guess there is a pattern here where we assume this implies a return. I am covering the bases now, going though all changes. Cascardo. > if (rt_entry_delete(plen + 32, &ip6, plen)) { > unixctl_command_reply(conn, "OK"); OK. > seq_change(tnl_conf_seq); > } else { > - unixctl_command_reply(conn, "Not found"); > + unixctl_command_reply_error(conn, "Not found"); > } > } > This is OK too. But should we do a return in these cases as well, establishing a pattern? The one in ovs_router_show is fine too. > @@ -347,7 +348,8 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED, > if (scan_ipv4_route(argv[1], &ip, &plen) && plen == 32) { > in6_addr_set_mapped_ipv4(&ip6, ip); > } else if (!(scan_ipv6_route(argv[1], &ip6, &plen) && plen == 128)) { > - unixctl_command_reply(conn, "Invalid parameters"); > + unixctl_command_reply_error(conn, "Invalid parameters"); > + return; > } > OK. > if (ovs_router_lookup(&ip6, iface, &gw)) { > @@ -358,7 +360,7 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED, > unixctl_command_reply(conn, ds_cstr(&ds)); OK. > ds_destroy(&ds); > } else { > - unixctl_command_reply(conn, "Not found"); > + unixctl_command_reply_error(conn, "Not found"); > } > } > This doesn't need the return, but see above about a pattern. > -- > 2.1.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Wed, Nov 11, 2015 at 12:19:25PM -0200, Thadeu Lima de Souza Cascardo wrote: > On Tue, Nov 10, 2015 at 04:13:47PM -0800, Ben Pfaff wrote: > > The _error version should be used to report errors. > > > > Also, add missing return in one error case. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > v1->v2: Add missing return in error case (thanks Cascardo!). > > > > lib/ovs-router.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > All other uses are fine, when considering only the context of the function they are called in. Many of them use the pattern of using return or goto out/exit in the same block as calling unixctl_command_reply{,_error}. No use of return when at the end of the function in a if/else if/else block. Cascardo.
On Wed, Nov 11, 2015 at 12:19:25PM -0200, Thadeu Lima de Souza Cascardo wrote: > On Tue, Nov 10, 2015 at 04:13:47PM -0800, Ben Pfaff wrote: > > The _error version should be used to report errors. > > > > Also, add missing return in one error case. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > v1->v2: Add missing return in error case (thanks Cascardo!). > > > > lib/ovs-router.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/lib/ovs-router.c b/lib/ovs-router.c > > index 2f093e8..619aa3a 100644 > > --- a/lib/ovs-router.c > > +++ b/lib/ovs-router.c > > @@ -275,7 +275,8 @@ ovs_router_add(struct unixctl_conn *conn, int argc, > > gw6 = in6addr_any; > > } > > } else { > > - unixctl_command_reply(conn, "Invalid parameters"); > > + unixctl_command_reply_error(conn, "Invalid parameters"); > > + return; > > This is OK, included in the first patch. > > > } > > ovs_router_insert__(plen + 32, &ip6, plen, argv[2], &gw6); > > unixctl_command_reply(conn, "OK"); > > OK. > > > @@ -293,13 +294,13 @@ ovs_router_del(struct unixctl_conn *conn, int argc OVS_UNUSED, > > in6_addr_set_mapped_ipv4(&ip6, ip); > > plen += 96; > > } else if (!scan_ipv6_route(argv[1], &ip6, &plen)) { > > - unixctl_command_reply(conn, "Invalid parameters"); > > + unixctl_command_reply_error(conn, "Invalid parameters"); > > } > > My bad. This should have that return too. That amounts to three > missing returns. I guess there is a pattern here where we assume this > implies a return. I am covering the bases now, going though all > changes. Thanks. I fixed this one too. (I should have looked carefully through the patch when you pointed out the first one. My mistake.) > Cascardo. > > > if (rt_entry_delete(plen + 32, &ip6, plen)) { > > unixctl_command_reply(conn, "OK"); > > OK. > > > seq_change(tnl_conf_seq); > > } else { > > - unixctl_command_reply(conn, "Not found"); > > + unixctl_command_reply_error(conn, "Not found"); > > } > > } > > > > This is OK too. But should we do a return in these cases as well, establishing a > pattern? I can see what you mean by a pattern, but I think that the result looks funny if I add a return here but not in the other fork of the 'if' statement. Anyway, I decided to only change the one case where it was needed. I posted a v3: http://openvswitch.org/pipermail/dev/2015-November/062425.html Thanks! Ben
diff --git a/lib/ovs-router.c b/lib/ovs-router.c index 2f093e8..619aa3a 100644 --- a/lib/ovs-router.c +++ b/lib/ovs-router.c @@ -275,7 +275,8 @@ ovs_router_add(struct unixctl_conn *conn, int argc, gw6 = in6addr_any; } } else { - unixctl_command_reply(conn, "Invalid parameters"); + unixctl_command_reply_error(conn, "Invalid parameters"); + return; } ovs_router_insert__(plen + 32, &ip6, plen, argv[2], &gw6); unixctl_command_reply(conn, "OK"); @@ -293,13 +294,13 @@ ovs_router_del(struct unixctl_conn *conn, int argc OVS_UNUSED, in6_addr_set_mapped_ipv4(&ip6, ip); plen += 96; } else if (!scan_ipv6_route(argv[1], &ip6, &plen)) { - unixctl_command_reply(conn, "Invalid parameters"); + unixctl_command_reply_error(conn, "Invalid parameters"); } if (rt_entry_delete(plen + 32, &ip6, plen)) { unixctl_command_reply(conn, "OK"); seq_change(tnl_conf_seq); } else { - unixctl_command_reply(conn, "Not found"); + unixctl_command_reply_error(conn, "Not found"); } } @@ -347,7 +348,8 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED, if (scan_ipv4_route(argv[1], &ip, &plen) && plen == 32) { in6_addr_set_mapped_ipv4(&ip6, ip); } else if (!(scan_ipv6_route(argv[1], &ip6, &plen) && plen == 128)) { - unixctl_command_reply(conn, "Invalid parameters"); + unixctl_command_reply_error(conn, "Invalid parameters"); + return; } if (ovs_router_lookup(&ip6, iface, &gw)) { @@ -358,7 +360,7 @@ ovs_router_lookup_cmd(struct unixctl_conn *conn, int argc OVS_UNUSED, unixctl_command_reply(conn, ds_cstr(&ds)); ds_destroy(&ds); } else { - unixctl_command_reply(conn, "Not found"); + unixctl_command_reply_error(conn, "Not found"); } }
The _error version should be used to report errors. Also, add missing return in one error case. Signed-off-by: Ben Pfaff <blp@ovn.org> --- v1->v2: Add missing return in error case (thanks Cascardo!). lib/ovs-router.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)