diff mbox series

[ovs-dev,08/11] ovn-nbctl: Propagate error thru the context.

Message ID 20180717133415.23781-9-jkbs@redhat.com
State Accepted
Headers show
Series Get rid of ctl_fatal() calls in ovn-nbctl (part 2) | expand

Commit Message

Jakub Sitnicki July 17, 2018, 1:34 p.m. UTC 
Instead of dying let the main loop handle the error.
This will allow us to report errors when running in daemon mode.

This is a result of applying the following semantic patch:

@@
identifier F;
identifier C;
identifier E;
@@
  static void F(struct ctl_context *C) {
<...
      if (E) {
-         ctl_fatal("%s", E);
+         C->error = E;
+         return;
      }
...>
  }

Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
---
 ovn/utilities/ovn-nbctl.c | 135 ++++++++++++++++++++++++++++++----------------
 1 file changed, 90 insertions(+), 45 deletions(-)

Comments

Ben Pfaff July 23, 2018, 10:36 p.m. UTC | #1 
On Tue, Jul 17, 2018 at 03:34:12PM +0200, Jakub Sitnicki wrote:
> Instead of dying let the main loop handle the error.
> This will allow us to report errors when running in daemon mode.
> 
> This is a result of applying the following semantic patch:
> 
> @@
> identifier F;
> identifier C;
> identifier E;
> @@
>   static void F(struct ctl_context *C) {
> <...
>       if (E) {
> -         ctl_fatal("%s", E);
> +         C->error = E;
> +         return;
>       }
> ...>
>   }
> 
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>

It's too bad that this generates code like this:

         char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], false, &ls);
         if (error) {
            ctx->error = error;
            return;
         }

instead of the simpler:

         ctx->error = ls_by_name_or_uuid(ctx, ctx->argv[1], false, &ls);
         if (ctx->error) {
            return;
         }

Oh well.
Ben Pfaff July 23, 2018, 10:38 p.m. UTC | #2 
On Tue, Jul 17, 2018 at 03:34:12PM +0200, Jakub Sitnicki wrote:
> Instead of dying let the main loop handle the error.
> This will allow us to report errors when running in daemon mode.
> 
> This is a result of applying the following semantic patch:
> 
> @@
> identifier F;
> identifier C;
> identifier E;
> @@
>   static void F(struct ctl_context *C) {
> <...
>       if (E) {
> -         ctl_fatal("%s", E);
> +         C->error = E;
> +         return;
>       }
> ...>
>   }
> 
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>

Thanks.

I adjusted this to change a couple of instances of:

    return;
}

by deleting the "return;".
Jakub Sitnicki July 24, 2018, 1:13 p.m. UTC | #3 
On Mon, 23 Jul 2018 15:36:28 -0700
Ben Pfaff <blp@ovn.org> wrote:

> On Tue, Jul 17, 2018 at 03:34:12PM +0200, Jakub Sitnicki wrote:
> > Instead of dying let the main loop handle the error.
> > This will allow us to report errors when running in daemon mode.
> > 
> > This is a result of applying the following semantic patch:
> > 
> > @@
> > identifier F;
> > identifier C;
> > identifier E;
> > @@
> >   static void F(struct ctl_context *C) {
> > <...
> >       if (E) {
> > -         ctl_fatal("%s", E);
> > +         C->error = E;
> > +         return;
> >       }  
> > ...>  
> >   }
> > 
> > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>  
> 
> It's too bad that this generates code like this:
> 
>          char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], false, &ls);
>          if (error) {
>             ctx->error = error;
>             return;
>          }
> 
> instead of the simpler:
> 
>          ctx->error = ls_by_name_or_uuid(ctx, ctx->argv[1], false, &ls);
>          if (ctx->error) {
>             return;
>          }
> 
> Oh well.

Let me see if I can come up with a semantic patch that corrects that.

-Jakub
diff mbox series

Patch

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index cf49a6b11..cf2b8eced 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -813,7 +813,8 @@  nbctl_show(struct ctl_context *ctx)
     if (ctx->argc == 2) {
         char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], false, &ls);
         if (error) {
-            ctl_fatal("%s", error);
+            ctx->error = error;
+            return;
         }
         if (ls) {
             print_ls(ls, &ctx->output);
@@ -828,7 +829,8 @@  nbctl_show(struct ctl_context *ctx)
     if (ctx->argc == 2) {
         char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], false, &lr);
         if (error) {
-            ctl_fatal("%s", error);
+            ctx->error = error;
+            return;
         }
         if (lr) {
             print_lr(lr, &ctx->output);
@@ -1126,7 +1128,8 @@  nbctl_lsp_del(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, ctx->argv[1], must_exist, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (!lsp) {
         return;
@@ -1158,7 +1161,8 @@  nbctl_lsp_list(struct ctl_context *ctx)
 
     char *error = ls_by_name_or_uuid(ctx, id, true, &ls);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     smap_init(&lsps);
@@ -1183,7 +1187,8 @@  nbctl_lsp_get_parent(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, ctx->argv[1], true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (lsp->parent_name) {
         ds_put_format(&ctx->output, "%s\n", lsp->parent_name);
@@ -1197,7 +1202,8 @@  nbctl_lsp_get_tag(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, ctx->argv[1], true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (lsp->n_tag > 0) {
         ds_put_format(&ctx->output, "%"PRId64"\n", lsp->tag[0]);
@@ -1212,7 +1218,8 @@  nbctl_lsp_set_addresses(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     int i;
@@ -1245,7 +1252,8 @@  nbctl_lsp_get_addresses(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     svec_init(&addresses);
@@ -1267,7 +1275,8 @@  nbctl_lsp_set_port_security(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     nbrec_logical_switch_port_set_port_security(lsp,
             (const char **) ctx->argv + 2, ctx->argc - 2);
@@ -1284,7 +1293,8 @@  nbctl_lsp_get_port_security(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     svec_init(&addrs);
     for (i = 0; i < lsp->n_port_security; i++) {
@@ -1305,7 +1315,8 @@  nbctl_lsp_get_up(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     ds_put_format(&ctx->output,
                   "%s\n", (lsp->up && *lsp->up) ? "up" : "down");
@@ -1336,12 +1347,14 @@  nbctl_lsp_set_enabled(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     bool enabled;
     error = parse_enabled(state, &enabled);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     nbrec_logical_switch_port_set_enabled(lsp, &enabled, 1);
 }
@@ -1354,7 +1367,8 @@  nbctl_lsp_get_enabled(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     ds_put_format(&ctx->output, "%s\n",
                   !lsp->enabled || *lsp->enabled ? "enabled" : "disabled");
@@ -1389,7 +1403,8 @@  nbctl_lsp_get_type(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     ds_put_format(&ctx->output, "%s\n", lsp->type);
 }
@@ -1404,7 +1419,8 @@  nbctl_lsp_set_options(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     for (i = 2; i < ctx->argc; i++) {
         char *key, *value;
@@ -1430,7 +1446,8 @@  nbctl_lsp_get_options(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     SMAP_FOR_EACH(node, &lsp->options) {
         ds_put_format(&ctx->output, "%s=%s\n", node->key, node->value);
@@ -1445,13 +1462,15 @@  nbctl_lsp_set_dhcpv4_options(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     const struct nbrec_dhcp_options *dhcp_opt = NULL;
     if (ctx->argc == 3 ) {
         error = dhcp_options_get(ctx, ctx->argv[2], true, &dhcp_opt);
         if (error) {
-            ctl_fatal("%s", error);
+            ctx->error = error;
+            return;
         }
     }
 
@@ -1475,13 +1494,15 @@  nbctl_lsp_set_dhcpv6_options(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     const struct nbrec_dhcp_options *dhcp_opt = NULL;
     if (ctx->argc == 3) {
         error = dhcp_options_get(ctx, ctx->argv[2], true, &dhcp_opt);
         if (error) {
-            ctl_fatal("%s", error);
+            ctx->error = error;
+            return;
         }
     }
 
@@ -1505,7 +1526,8 @@  nbctl_lsp_get_dhcpv4_options(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (lsp->dhcpv4_options) {
         ds_put_format(&ctx->output, UUID_FMT " (%s)\n",
@@ -1522,7 +1544,8 @@  nbctl_lsp_get_dhcpv6_options(struct ctl_context *ctx)
 
     char *error = lsp_by_name_or_uuid(ctx, id, true, &lsp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (lsp->dhcpv6_options) {
         ds_put_format(&ctx->output, UUID_FMT " (%s)\n",
@@ -1623,7 +1646,8 @@  nbctl_acl_list(struct ctl_context *ctx)
 
     char *error = acl_cmd_get_pg_or_ls(ctx, &ls, &pg);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     size_t n_acls = pg ? pg->n_acls : ls->n_acls;
@@ -1802,7 +1826,8 @@  nbctl_acl_del(struct ctl_context *ctx)
 
     char *error = acl_cmd_get_pg_or_ls(ctx, &ls, &pg);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     if (ctx->argc == 2) {
@@ -1821,7 +1846,8 @@  nbctl_acl_del(struct ctl_context *ctx)
     const char *direction;
     error = parse_direction(ctx->argv[2], &direction);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     size_t n_acls = pg ? pg->n_acls : ls->n_acls;
@@ -1852,7 +1878,8 @@  nbctl_acl_del(struct ctl_context *ctx)
     int64_t priority;
     error = parse_priority(ctx->argv[3], &priority);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     if (ctx->argc == 4) {
@@ -1892,7 +1919,8 @@  nbctl_qos_list(struct ctl_context *ctx)
 
     char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], true, &ls);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     qos_rules = xmalloc(sizeof *qos_rules * ls->n_qos_rules);
@@ -2046,7 +2074,8 @@  nbctl_qos_del(struct ctl_context *ctx)
     const struct nbrec_logical_switch *ls;
     char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], true, &ls);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     if (ctx->argc == 2) {
@@ -2086,7 +2115,8 @@  nbctl_qos_del(struct ctl_context *ctx)
     int64_t priority;
     error = parse_priority(ctx->argv[3], &priority);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     if (ctx->argc == 4) {
@@ -2433,7 +2463,8 @@  nbctl_lr_lb_del(struct ctl_context *ctx)
     const struct nbrec_load_balancer *del_lb;
     char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     if (ctx->argc == 2) {
@@ -2446,7 +2477,8 @@  nbctl_lr_lb_del(struct ctl_context *ctx)
 
     error = lb_by_name_or_uuid(ctx, ctx->argv[2], true, &del_lb);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     for (size_t i = 0; i < lr->n_load_balancer; i++) {
         const struct nbrec_load_balancer *lb
@@ -2484,7 +2516,8 @@  nbctl_lr_lb_list(struct ctl_context *ctx)
 
     char *error = lr_by_name_or_uuid(ctx, lr_name, true, &lr);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     for (int i = 0; i < lr->n_load_balancer; i++) {
         const struct nbrec_load_balancer *lb
@@ -2554,7 +2587,8 @@  nbctl_ls_lb_del(struct ctl_context *ctx)
     const struct nbrec_load_balancer *del_lb;
     char *error = ls_by_name_or_uuid(ctx, ctx->argv[1], true, &ls);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     if (ctx->argc == 2) {
@@ -2567,7 +2601,8 @@  nbctl_ls_lb_del(struct ctl_context *ctx)
 
     error = lb_by_name_or_uuid(ctx, ctx->argv[2], true, &del_lb);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     for (size_t i = 0; i < ls->n_load_balancer; i++) {
         const struct nbrec_load_balancer *lb
@@ -2605,7 +2640,8 @@  nbctl_ls_lb_list(struct ctl_context *ctx)
 
     char *error = ls_by_name_or_uuid(ctx, ls_name, true, &ls);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     for (int i = 0; i < ls->n_load_balancer; i++) {
         const struct nbrec_load_balancer *lb
@@ -2762,7 +2798,8 @@  nbctl_dhcp_options_set_options(struct ctl_context *ctx)
     const struct nbrec_dhcp_options *dhcp_opts;
     char *error = dhcp_options_get(ctx, ctx->argv[1], true, &dhcp_opts);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     struct smap dhcp_options = SMAP_INITIALIZER(&dhcp_options);
@@ -2786,7 +2823,8 @@  nbctl_dhcp_options_get_options(struct ctl_context *ctx)
     const struct nbrec_dhcp_options *dhcp_opts;
     char *error = dhcp_options_get(ctx, ctx->argv[1], true, &dhcp_opts);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     struct smap_node *node;
@@ -2804,7 +2842,8 @@  nbctl_dhcp_options_del(struct ctl_context *ctx)
 
     char *error = dhcp_options_get(ctx, id, must_exist, &dhcp_opts);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (!dhcp_opts) {
         return;
@@ -3265,7 +3304,8 @@  nbctl_lr_nat_list(struct ctl_context *ctx)
     const struct nbrec_logical_router *lr;
     char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     struct smap lr_nats = SMAP_INITIALIZER(&lr_nats);
@@ -3422,7 +3462,8 @@  nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx)
     const struct nbrec_gateway_chassis *existing_gc;
     error = gc_by_name_or_uuid(ctx, gc_name, false, &existing_gc);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (existing_gc) {
         nbrec_gateway_chassis_set_priority(existing_gc, priority);
@@ -3714,7 +3755,8 @@  nbctl_lrp_del(struct ctl_context *ctx)
 
     char *error = lrp_by_name_or_uuid(ctx, ctx->argv[1], must_exist, &lrp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (!lrp) {
         return;
@@ -3747,7 +3789,8 @@  nbctl_lrp_list(struct ctl_context *ctx)
 
     char *error = lr_by_name_or_uuid(ctx, id, true, &lr);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     smap_init(&lrps);
@@ -3800,7 +3843,8 @@  nbctl_lrp_get_enabled(struct ctl_context *ctx)
 
     char *error = lrp_by_name_or_uuid(ctx, id, true, &lrp);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
     if (!lrp) {
         return;
@@ -3883,7 +3927,8 @@  nbctl_lr_route_list(struct ctl_context *ctx)
 
     char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
     if (error) {
-        ctl_fatal("%s", error);
+        ctx->error = error;
+        return;
     }
 
     ipv4_routes = xmalloc(sizeof *ipv4_routes * lr->n_static_routes);