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 <[email protected]>
> > ---
> > 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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
