Message ID | 20180702105019.10345-2-jkbs@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | Don't use ctl_fatal() in command handlers from db-ctl-base | expand |
Bleep bloop. Greetings Jakub Sitnicki, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #62 FILE: lib/db-ctl-base.c:988: die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); WARNING: Line is 80 characters long (recommended limit is 79) #71 FILE: lib/db-ctl-base.c:1077: die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); WARNING: Line is 80 characters long (recommended limit is 79) #80 FILE: lib/db-ctl-base.c:1150: die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); Lines checked: 87, Warnings: 3, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
On Mon, 2 Jul 2018 06:55:57 -0400 0-day Robot <robot@bytheb.org> wrote: > Bleep bloop. Greetings Jakub Sitnicki, I am a robot and I have tried out your patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > checkpatch: > WARNING: Line is 80 characters long (recommended limit is 79) > #62 FILE: lib/db-ctl-base.c:988: > die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); > > WARNING: Line is 80 characters long (recommended limit is 79) > #71 FILE: lib/db-ctl-base.c:1077: > die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); > > WARNING: Line is 80 characters long (recommended limit is 79) > #80 FILE: lib/db-ctl-base.c:1150: > die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); > > Lines checked: 87, Warnings: 3, Errors: 0 Thanks, robot! Patch 15/23 removes the offending lines. Hopefully the warnings can be ignored. BTW. To anyone reading the series - cover letter for this series is coming. It got detained and needs to be approved by the moderator for some reason. -Jakub
Hi Jakub, Your cover letter still hasn't come through, so feel free to tell me if my comments here are misguided. I'm guessing that you did not remove ctl_fatal() calls from parse_commands() and ctl_parse_commands() because there is no ctl_context present. However, it's not clear why the ctl_fatal() call in cmd_add() was not converted to use ctl_error() + return. I think patches 15 and 19 could be combined into one. You *could* combine patches 1-13 into one patch, 16-18 into one patch, and 20-23 in one patch, but I don't feel strongly enough about this to make you have to re-do the whole thing. On 07/02/2018 06:49 AM, Jakub Sitnicki wrote: > Return the error message to the caller instead of reporting it and dying > so that the caller can handle the error without terminating the process > if needed. > > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com> > --- > lib/db-ctl-base.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c > index 0e26f09cc..d589df487 100644 > --- a/lib/db-ctl-base.c > +++ b/lib/db-ctl-base.c > @@ -918,7 +918,8 @@ cmd_get(struct ctl_context *ctx) > } > } > > -static void > +/* Returns NULL on success or malloc()'ed error message on failure. */ > +static char * OVS_WARN_UNUSED_RESULT > parse_column_names(const char *column_names, > const struct ovsdb_idl_table_class *table, > const struct ovsdb_idl_column ***columnsp, > @@ -951,7 +952,12 @@ parse_column_names(const char *column_names, > if (!strcasecmp(column_name, "_uuid")) { > column = NULL; > } else { > - die_if_error(get_column(table, column_name, &column)); > + char *error = get_column(table, column_name, &column); > + if (error) { > + free(columns); > + free(s); > + return error; > + } > } > if (n_columns >= allocated_columns) { > columns = x2nrealloc(columns, &allocated_columns, > @@ -962,11 +968,12 @@ parse_column_names(const char *column_names, > free(s); > > if (!n_columns) { > - ctl_fatal("must specify at least one column name"); > + return xstrdup("must specify at least one column name"); > } > } > *columnsp = columns; > *n_columnsp = n_columns; > + return NULL; > } > > static void > @@ -978,7 +985,7 @@ pre_list_columns(struct ctl_context *ctx, > size_t n_columns; > size_t i; > > - parse_column_names(column_names, table, &columns, &n_columns); > + die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); > for (i = 0; i < n_columns; i++) { > if (columns[i]) { > ovsdb_idl_add_column(ctx->idl, columns[i]); > @@ -1067,7 +1074,7 @@ cmd_list(struct ctl_context *ctx) > int i; > > table = get_table(table_name); > - parse_column_names(column_names, table, &columns, &n_columns); > + die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); > out = ctx->table = list_make_table(columns, n_columns); > if (ctx->argc > 2) { > for (i = 2; i < ctx->argc; i++) { > @@ -1140,7 +1147,7 @@ cmd_find(struct ctl_context *ctx) > size_t n_columns; > > table = get_table(table_name); > - parse_column_names(column_names, table, &columns, &n_columns); > + die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); > out = ctx->table = list_make_table(columns, n_columns); > for (row = ovsdb_idl_first_row(ctx->idl, table); row; > row = ovsdb_idl_next_row(row)) { >
Hi Mark, Thanks for starting the review. On Mon, 2 Jul 2018 10:55:01 -0400 Mark Michelson <mmichels@redhat.com> wrote: > Your cover letter still hasn't come through, so feel free to tell me if > my comments here are misguided. Looks like the cover letter has arrived now. > I'm guessing that you did not remove ctl_fatal() calls from > parse_commands() and ctl_parse_commands() because there is no > ctl_context present. However, it's not clear why the ctl_fatal() call in > cmd_add() was not converted to use ctl_error() + return. I left ctl_parse_commands() & parse_commands() as is for now because I'm not sure yet if I will end up using them for the ovn-nbctl daemon mode in their current form. cmd_add(), OTOH, is an oversight. Thanks for pointing it out. Let me correct that in v2. > I think patches 15 and 19 could be combined into one. > > You *could* combine patches 1-13 into one patch, 16-18 into one patch, > and 20-23 in one patch, but I don't feel strongly enough about this to > make you have to re-do the whole thing. Squashing patches together is not a problem. It depends on what what we agree on to be one logical change. I find smaller patches easier to review without losing the context midway through it. Not to the point when the patch appears disjointed, though. Thanks, Jakub
diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c index 0e26f09cc..d589df487 100644 --- a/lib/db-ctl-base.c +++ b/lib/db-ctl-base.c @@ -918,7 +918,8 @@ cmd_get(struct ctl_context *ctx) } } -static void +/* Returns NULL on success or malloc()'ed error message on failure. */ +static char * OVS_WARN_UNUSED_RESULT parse_column_names(const char *column_names, const struct ovsdb_idl_table_class *table, const struct ovsdb_idl_column ***columnsp, @@ -951,7 +952,12 @@ parse_column_names(const char *column_names, if (!strcasecmp(column_name, "_uuid")) { column = NULL; } else { - die_if_error(get_column(table, column_name, &column)); + char *error = get_column(table, column_name, &column); + if (error) { + free(columns); + free(s); + return error; + } } if (n_columns >= allocated_columns) { columns = x2nrealloc(columns, &allocated_columns, @@ -962,11 +968,12 @@ parse_column_names(const char *column_names, free(s); if (!n_columns) { - ctl_fatal("must specify at least one column name"); + return xstrdup("must specify at least one column name"); } } *columnsp = columns; *n_columnsp = n_columns; + return NULL; } static void @@ -978,7 +985,7 @@ pre_list_columns(struct ctl_context *ctx, size_t n_columns; size_t i; - parse_column_names(column_names, table, &columns, &n_columns); + die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); for (i = 0; i < n_columns; i++) { if (columns[i]) { ovsdb_idl_add_column(ctx->idl, columns[i]); @@ -1067,7 +1074,7 @@ cmd_list(struct ctl_context *ctx) int i; table = get_table(table_name); - parse_column_names(column_names, table, &columns, &n_columns); + die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); out = ctx->table = list_make_table(columns, n_columns); if (ctx->argc > 2) { for (i = 2; i < ctx->argc; i++) { @@ -1140,7 +1147,7 @@ cmd_find(struct ctl_context *ctx) size_t n_columns; table = get_table(table_name); - parse_column_names(column_names, table, &columns, &n_columns); + die_if_error(parse_column_names(column_names, table, &columns, &n_columns)); out = ctx->table = list_make_table(columns, n_columns); for (row = ovsdb_idl_first_row(ctx->idl, table); row; row = ovsdb_idl_next_row(row)) {
Return the error message to the caller instead of reporting it and dying so that the caller can handle the error without terminating the process if needed. Signed-off-by: Jakub Sitnicki <jkbs@redhat.com> --- lib/db-ctl-base.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)