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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev