diff mbox

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

Message ID 1446851309-18383-1-git-send-email-blp@ovn.org
State Superseded
Headers show

Commit Message

Ben Pfaff Nov. 6, 2015, 11:08 p.m. UTC 
The _error version should be used to report errors.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ovs-router.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Thadeu Lima de Souza Cascardo Nov. 9, 2015, 12:51 p.m. UTC | #1 
On Fri, Nov 06, 2015 at 03:08:29PM -0800, Ben Pfaff wrote:
> The _error version should be used to report errors.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  lib/ovs-router.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 2f093e8..50737a2 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,7 @@ 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");
>      }

This should have a return here as well. Otherwise, we may query for an invalid
address, and would assert at the second unixctl_command_reply.

Cascardo.

>  
>      if (ovs_router_lookup(&ip6, iface, &gw)) {
> @@ -358,7 +359,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");
>      }
>  }
>  
> -- 
> 2.1.3
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Ben Pfaff Nov. 11, 2015, 12:14 a.m. UTC | #2 
On Mon, Nov 09, 2015 at 10:51:48AM -0200, Thadeu Lima de Souza Cascardo wrote:
> On Fri, Nov 06, 2015 at 03:08:29PM -0800, Ben Pfaff wrote:
> > The _error version should be used to report errors.
> > 
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  lib/ovs-router.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> > index 2f093e8..50737a2 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,7 @@ 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");
> >      }
> 
> This should have a return here as well. Otherwise, we may query for an invalid
> address, and would assert at the second unixctl_command_reply.

Thanks, I've fixed that for v2:
        http://openvswitch.org/pipermail/dev/2015-November/062087.html
diff mbox

Patch

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 2f093e8..50737a2 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,7 @@  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");
     }
 
     if (ovs_router_lookup(&ip6, iface, &gw)) {
@@ -358,7 +359,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");
     }
 }