| Message ID | 20180717133415.23781-2-jkbs@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Get rid of ctl_fatal() calls in ovn-nbctl (part 2) | expand |
(Cover-letter awaits moderator approval. Reposting here.) This is a continuation of an earlier series that aims to replace calls to ctl_fatal() in command handlers in ovn-nbctl. The motivation is to handle errors gracefully when running commands in daemon mode because as a long-lived process we shouldn't terminate on errors that we can recover from. After this series there are no more ctl_fatal() calls in ovn-nbctl that affect the daemon mode. The only remaining function left to convert is the commands parser in db-ctl-base module (ctl_parse_commands()), which I intend to deal with separately. Either as a part of ovn-nbctl daemon series (already in review [1]), or as a follow-up to it. Thanks, Jakub [1] https://patchwork.ozlabs.org/project/openvswitch/list/?series=55472 Jakub Sitnicki (11): ovn-nbctl: Don't die in pg_by_name_or_uuid(). ovn-nbctl: Don't die in gc_by_name_or_uuid(). ovn-nbctl: Don't die in lsp_to_ls(). ovn-nbctl: Don't die in lrp_to_lr(). ovn-nbctl: Don't die in parse_priority(). ovn-nbctl: Don't die in parse_direction(). ovn-nbctl: Don't die in dhcp_options_get(). ovn-nbctl: Propagate error thru the context. ovn-nbctl: Use ctl_error() in command handlers. ovn-nbctl: Remove pointless "return;" at ends of functions. ovn-nbctl: Fix mem leak in nbctl_lrp_set_gateway_chassis(). ovn/utilities/ovn-nbctl.c | 360 ++++++++++++++++++++++++++++++++-------------- tests/ovn-nbctl.at | 5 + 2 files changed, 260 insertions(+), 105 deletions(-)
Looks good! Acked-by: Mark Michelson <mmichels@redhat.com> On 07/17/2018 09:36 AM, Jakub Sitnicki wrote: > (Cover-letter awaits moderator approval. Reposting here.) > > This is a continuation of an earlier series that aims to replace calls > to ctl_fatal() in command handlers in ovn-nbctl. The motivation is to > handle errors gracefully when running commands in daemon mode because > as a long-lived process we shouldn't terminate on errors that we can > recover from. > > After this series there are no more ctl_fatal() calls in ovn-nbctl that > affect the daemon mode. The only remaining function left to convert is > the commands parser in db-ctl-base module (ctl_parse_commands()), which > I intend to deal with separately. Either as a part of ovn-nbctl daemon > series (already in review [1]), or as a follow-up to it. > > Thanks, > Jakub > > [1] https://patchwork.ozlabs.org/project/openvswitch/list/?series=55472 > > Jakub Sitnicki (11): > ovn-nbctl: Don't die in pg_by_name_or_uuid(). > ovn-nbctl: Don't die in gc_by_name_or_uuid(). > ovn-nbctl: Don't die in lsp_to_ls(). > ovn-nbctl: Don't die in lrp_to_lr(). > ovn-nbctl: Don't die in parse_priority(). > ovn-nbctl: Don't die in parse_direction(). > ovn-nbctl: Don't die in dhcp_options_get(). > ovn-nbctl: Propagate error thru the context. > ovn-nbctl: Use ctl_error() in command handlers. > ovn-nbctl: Remove pointless "return;" at ends of functions. > ovn-nbctl: Fix mem leak in nbctl_lrp_set_gateway_chassis(). > > ovn/utilities/ovn-nbctl.c | 360 > ++++++++++++++++++++++++++++++++-------------- > tests/ovn-nbctl.at | 5 + 2 files changed, 260 insertions(+), > 105 deletions(-) > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index d07524697..0cd11e845 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -609,10 +609,12 @@ lb_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist, return NULL; } -static const struct nbrec_port_group * -pg_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist) +static char * OVS_WARN_UNUSED_RESULT +pg_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist, + const struct nbrec_port_group **pg_p) { const struct nbrec_port_group *pg = NULL; + *pg_p = NULL; struct uuid pg_uuid; bool is_uuid = uuid_from_string(&pg_uuid, id); @@ -632,11 +634,12 @@ pg_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool must_exist) } if (!pg && must_exist) { - ctl_fatal("%s: port group %s not found", id, - is_uuid ? "UUID" : "name"); + return xasprintf("%s: port group %s not found", id, + is_uuid ? "UUID" : "name"); } - return pg; + *pg_p = pg; + return NULL; } static void @@ -1559,7 +1562,10 @@ acl_cmd_get_pg_or_ls(struct ctl_context *ctx, char *error; if (!opt_type) { - *pg = pg_by_name_or_uuid(ctx, ctx->argv[1], false); + error = pg_by_name_or_uuid(ctx, ctx->argv[1], false, pg); + if (error) { + return error; + } error = ls_by_name_or_uuid(ctx, ctx->argv[1], false, ls); if (error) { return error; @@ -1574,7 +1580,10 @@ acl_cmd_get_pg_or_ls(struct ctl_context *ctx, ctx->argv[1]); } } else if (!strcmp(opt_type, "port-group")) { - *pg = pg_by_name_or_uuid(ctx, ctx->argv[1], true); + error = pg_by_name_or_uuid(ctx, ctx->argv[1], true, pg); + if (error) { + return error; + } *ls = NULL; } else if (!strcmp(opt_type, "switch")) { error = ls_by_name_or_uuid(ctx, ctx->argv[1], true, ls); diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 73a61a4be..9f6a2783b 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -234,6 +234,11 @@ OVN_NBCTL_TEST_ACL([ls1], [--type=switch]) AT_CHECK([ovn-nbctl create port_group name=pg0], [0], [ignore]) OVN_NBCTL_TEST_ACL([pg0], [--type=port-group]) +dnl Test when port group doesn't exist +AT_CHECK([ovn-nbctl --type=port-group acl-add pg1 to-lport 100 ip drop], [1], [], [dnl +ovn-nbctl: pg1: port group name not found +]) + dnl Test when same name exists in logical switches and portgroups AT_CHECK([ovn-nbctl create port_group name=ls0], [0], [ignore]) AT_CHECK([ovn-nbctl acl-add ls0 to-lport 100 ip drop], [1], [], [stderr])
Let the caller handle the error. This prepares us for reporting errors in daemon mode. Also, extend the tests to cover this error path. Signed-off-by: Jakub Sitnicki <jkbs@redhat.com> --- ovn/utilities/ovn-nbctl.c | 23 ++++++++++++++++------- tests/ovn-nbctl.at | 5 +++++ 2 files changed, 21 insertions(+), 7 deletions(-)