[ovs-dev] [PATCH 07/23] db-ctl-base: Don't die in get_row_by_id() on multiple matches.

[Jakub Sitnicki]

On Mon, 2 Jul 2018 10:44:17 -0400
Mark Michelson <mmichels at redhat.com> wrote:

> I'm not sure why you went with a different approach on this compared to 
> the other functions. I would have expected you would change 
> get_row_by_id() to return a string (NULL if no error, otherwise an error 
> string). Then return the row as an output parameter.

get_row_by_id() is special in a sense that it signals an error in
multiple places (everywhere we return NULL) but reports an error only
in one place (the ctl_fatal() call that was removed).

We could go with the other approach here and return an error message by
introducing meaningful error messages for all failure return points.

Going with 'multiple_matches' param was a subjective decision on what
seemed easier at that point.

Although now I'm no longer convinced that it was for the best. Perhaps
consistent error reporting would be better. Let me try it out.

Thanks,
Jakub

> 
> On 07/02/2018 06:50 AM, Jakub Sitnicki wrote:
> > Signal that multiple rows match the record identifier via a new output
> > parameter instead of reporting the problem and dying, so that the caller
> > can handle the error without terminating the process if needed.
> > 
> > Signed-off-by: Jakub Sitnicki <jkbs at redhat.com>
> > ---
> >   lib/db-ctl-base.c | 23 ++++++++++++++++++-----
> >   1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> > index e8f66811a..34daaef68 100644
> > --- a/lib/db-ctl-base.c
> > +++ b/lib/db-ctl-base.c
> > @@ -277,8 +277,11 @@ record_id_equals(const union ovsdb_atom *name, enum ovsdb_atomic_type type,
> >   static const struct ovsdb_idl_row *
> >   get_row_by_id(struct ctl_context *ctx,
> >                 const struct ovsdb_idl_table_class *table,
> > -              const struct ctl_row_id *id, const char *record_id)
> > +              const struct ctl_row_id *id, const char *record_id,
> > +              bool *multiple_matches)
> >   {
> > +    ovs_assert(multiple_matches);
> > +    *multiple_matches = false;
> >   
> >       if (!id->name_column) {
> >           return NULL;
> > @@ -336,8 +339,8 @@ get_row_by_id(struct ctl_context *ctx,
> >           /* If the name equals 'record_id', take it. */
> >           if (record_id_equals(name, name_type, record_id)) {
> >               if (referrer) {
> > -                ctl_fatal("multiple rows in %s match \"%s\"",
> > -                          id_table->name, record_id);
> > +                *multiple_matches = true;
> > +                return NULL;
> >               }
> >               referrer = row;
> >           }
> > @@ -386,8 +389,18 @@ ctl_get_row(struct ctl_context *ctx,
> >           const struct ctl_table_class *ctl_class
> >               = &ctl_classes[table - idl_classes];
> >           for (int i = 0; i < ARRAY_SIZE(ctl_class->row_ids); i++) {
> > -            row = get_row_by_id(ctx, table, &ctl_class->row_ids[i],
> > -                                record_id);
> > +            const struct ctl_row_id *id = &ctl_class->row_ids[i];
> > +            bool multiple_matches;
> > +
> > +            row = get_row_by_id(ctx, table, id, record_id, &multiple_matches);
> > +            if (multiple_matches) {
> > +                const struct ovsdb_idl_class *class =
> > +                    ovsdb_idl_get_class(ctx->idl);
> > +                const struct ovsdb_idl_table_class *table_class =
> > +                    ovsdb_idl_table_class_from_column(class, id->name_column);
> > +                ctl_fatal("multiple rows in %s match \"%s\"",
> > +                          table_class->name, record_id);
> > +            }
> >               if (row) {
> >                   break;
> >               }
> >   
>