[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