[ovs-dev,01/23] db-ctl-base: Don't die in parse_column_names() on error.

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(-)

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