[ovs-dev] [PATCH] ovsdb-idl : Add APIs to query if a table and a column is present or not.

Numan Siddique numans at ovn.org
Fri Aug 20 15:59:22 UTC 2021


On Fri, Aug 20, 2021 at 10:23 AM Michael Santana <msantana at redhat.com> wrote:
>
> On Wed, Aug 18, 2021 at 8:43 PM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > This patch adds 2 new APIs in the ovsdb-idl client library
> >  - ovsdb_idl_has_table() and ovsdb_idl_has_column_in_table() to
> > query if a table and a column is present in the IDL or not.  This
> > is required for scenarios where the server schema is old and
> > missing a table or column and the client (built with a new schema
> > version) does a transaction with the missing table or column.  This
> > results in a continuous loop of transaction failures.
> >
> > OVN would require the API - ovsdb_idl_has_table() to address this issue
> > when an old ovsdb-server is used (OVS 2.11) which has the 'datapath'
> > table missing.  A recent commit in OVN creates a 'datapath' row
> > in the local ovsdb-server.  ovsdb_idl_has_column_in_table() would be
> > useful to have.
> >
> > Related issue: https://bugzilla.redhat.com/show_bug.cgi?id=1992705
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > ---
> >  lib/ovsdb-idl.c | 25 +++++++++++++++++++++++++
> >  lib/ovsdb-idl.h |  4 ++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index 2198c69c6..c5795fb7f 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -4256,3 +4256,28 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop)
> >
> >      return retval;
> >  }
> > +
> > +bool
> > +ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name)
> > +{
> > +    struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name,
> > +                                                    table_name);
> > +    return table ? true: false;
> > +}
> > +
> > +bool
> > +ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name,
> > +                              const char *column_name)
> > +{
> > +    struct ovsdb_idl_table *table = shash_find_data(&idl->table_by_name,
> > +                                                    table_name);
> > +    if (!table) {
> > +        return false;
> > +    }
> > +
> > +    if (shash_find_data(&table->columns, column_name)) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> Why not use ovsdb_idl_has_table you created and make this function
> more concise? I would have done:
>
> if (ovsdb_idl_has_table(idl, table_name) &&
>     shash_find_data(&table->columns, column_name))

Thanks for the reviews.

This would result in one extra lookup in the shash right ?


>     return true;
> return false;
>
> I dont know if this breaks any coding conventions though :)


Just using ovsdb_idl_has_table() would not be sufficient right ?
We still need to get the table from the 'idl->table_from_name' shash.

I thought about doing something like below

-----------------
static struct ovsdb_idl_table*
ovsdb_idl_get_table(struct ovsdb_idl *idl, const char *table_name)
{
    return shash_find_data(&idl->table_by_name, table_name);
}

bool
ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name)
{
    return ovsdb_idl_get_table(idl, table_name) ? true: false;
}

bool
ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl, const char *table_name,
                                                     const char *column_name)
{
    struct ovsdb_idl_table *table = ovsdb_idl_get_table(idl, table_name);
    if (table && shash_find_data(&table->columns, column_name)) {
       return true;
    }
    return false;
}
------------------------

Then I dropped it as this would just add an extra function overhead
just to do 'shash_find_data()'.

I'm fine if this improves readability.

Thanks
Numan


>
> Otherwise LGTM
> > +}
> > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> > index d93483245..216db4c6d 100644
> > --- a/lib/ovsdb-idl.h
> > +++ b/lib/ovsdb-idl.h
> > @@ -474,6 +474,10 @@ void ovsdb_idl_cursor_next_eq(struct ovsdb_idl_cursor *);
> >
> >  struct ovsdb_idl_row *ovsdb_idl_cursor_data(struct ovsdb_idl_cursor *);
> >
> > +bool ovsdb_idl_has_table(struct ovsdb_idl *idl, const char *table_name);
> > +bool ovsdb_idl_has_column_in_table(struct ovsdb_idl *idl,
> > +                                   const char *table_name,
> > +                                   const char *column_name);
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list