[ovs-dev] [PATCH v2] ovs-router: Report ovs/route/add errors as errors.

[Thadeu Lima de Souza Cascardo]

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 at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev