[ovs-dev] [PATCH v2 08/26] ovn-sbctl: Add daemon support.

[Ben Pfaff]

On Fri, Apr 16, 2021 at 12:05:12PM -0400, Mark Michelson wrote:
> On 4/1/21 7:20 PM, Ben Pfaff wrote:
> > +static bool
> > +check_conflicts(struct ctl_context *ctx, const char *name, char *msg)
> > +{
> > +    struct sbctl_context *sbctl_ctx = sbctl_context_get(ctx);
> >       if (shash_find(&sbctl_ctx->chassis, name)) {
> > -        ctl_fatal("%s because a chassis named %s already exists",
> > -                    msg, name);
> > +        ctl_error(&sbctl_ctx->base,
> > +                  "%s because a chassis named %s already exists",
> > +                  msg, name);
> 
> I think this branch should free msg as well. It's more important in daemon
> mode since the program isn't exiting immediately in this case.
> 
> Since check_conflicts won't spontaneously exit now, it probably would make
> things easier for the caller of check_conflicts() to be responsible for
> freeing msg. This way, msg is allocated and freed in the same scope, and it
> gives the option of using a non dynamically allocated msg if desired.

Thanks, good points.  It turned out check_conflicts() was only used in
one place and that the code got nicer if we just inlined it, like this:

@@ -266,21 +266,6 @@ sbctl_ctx_destroy(struct ctl_context *ctx)
     free(ctx);
 }
 
-static bool
-check_conflicts(struct ctl_context *ctx, const char *name, char *msg)
-{
-    struct sbctl_context *sbctl_ctx = sbctl_context_get(ctx);
-    if (shash_find(&sbctl_ctx->chassis, name)) {
-        ctl_error(&sbctl_ctx->base,
-                  "%s because a chassis named %s already exists",
-                  msg, name);
-        return false;
-    }
-    free(msg);
-
-    return true;
-}
-
 static struct sbctl_chassis *
 find_chassis(struct ctl_context *ctx, const char *name, bool must_exist)
 {
@@ -389,15 +374,18 @@ cmd_chassis_add(struct ctl_context *ctx)
     encap_types = ctx->argv[2];
     encap_ip = ctx->argv[3];
 
-    if (may_exist) {
-        struct sbctl_chassis *sbctl_ch = find_chassis(ctx, ch_name, false);
-        if (sbctl_ch) {
+    if (find_chassis(ctx, ch_name, false)) {
+        if (may_exist) {
             return;
         }
     }
-    if (!check_conflicts(ctx, ch_name,
-                         xasprintf("cannot create a chassis named %s",
-                                   ch_name))) {
+
+    struct sbctl_context *sbctl_ctx = sbctl_context_get(ctx);
+    if (shash_find(&sbctl_ctx->chassis, ch_name)) {
+        if (!may_exist) {
+            ctl_error(ctx, "cannot create a chassis named %s because a "
+                      "chassis named %s already exists", ch_name, ch_name);
+        }
         return;
     }
 
> > @@ -672,17 +464,21 @@ cmd_lsp_bind(struct ctl_context *ctx)
> >       lport_name = ctx->argv[1];
> >       ch_name = ctx->argv[2];
> > -    sbctl_context_populate_cache(ctx);
> > -    sbctl_bd = find_port_binding(sbctl_ctx, lport_name, true);
> > -    sbctl_ch = find_chassis(sbctl_ctx, ch_name, true);
> > +    sbctl_bd = find_port_binding(ctx, lport_name, true);
> > +    if (!sbctl_ctx) {
> 
> Should this be
> 
>     if (!sbctl_bd) {
> 
> ?

Yes, thanks, fixed.