[ovs-dev] [PATCH 2/3] db-ctl-base: Do not expose get_table() API

Andy Zhou azhou at nicira.com
Thu Jul 9 20:02:38 UTC 2015


On Wed, Jul 8, 2015 at 11:44 PM, Alex Wang <alexw at nicira.com> wrote:
>
>
> On Tue, Jul 7, 2015 at 8:08 PM, Andy Zhou <azhou at nicira.com> wrote:
>>
>> Both get_table() and set_cloum() APIs are mostly used within db-ctl-base
>> library. This patch makes both private to the library.
>>
>> Add a new ctl_set_colum() API for library client.
>>
>> The changes are cleanups. No functional changes.
>>
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>> ---
>>  lib/db-ctl-base.c     | 20 ++++++++++++++++++--
>>  lib/db-ctl-base.h     | 12 +++---------
>>  utilities/ovs-vsctl.c |  4 ++--
>>  3 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
>> index 10884b4..4092f2b 100644
>> --- a/lib/db-ctl-base.c
>> +++ b/lib/db-ctl-base.c
>> @@ -46,7 +46,16 @@ VLOG_DEFINE_THIS_MODULE(db_ctl_base);
>>  struct ovsdb_idl *the_idl;
>>  struct ovsdb_idl_txn *the_idl_txn;
>>
>> +/* Represents all tables in the schema.  User must define 'tables'
>> + * in implementation and supply via clt_init().  The definition must end
>> + * with an all-NULL entry. */
>> +const struct ctl_table_class *tables;
>> +
>>  static struct shash all_commands = SHASH_INITIALIZER(&all_commands);
>> +static const struct ctl_table_class *get_table(const char *table_name);
>> +static void set_column(const struct ctl_table_class *,
>> +                       const struct ovsdb_idl_row *, const char *,
>> +                       struct ovsdb_symbol_table *);
>>
>>
>>  static struct option *
>> @@ -1990,7 +1999,7 @@ ctl_context_done(struct ctl_context *ctx,
>>
>>  /* Finds and returns the "struct ctl_table_class *" with 'table_name' by
>>   * searching the 'tables'. */
>> -const struct ctl_table_class *
>> +static const struct ctl_table_class *
>>  get_table(const char *table_name)
>>  {
>>      const struct ctl_table_class *table;
>> @@ -2018,7 +2027,7 @@ get_table(const char *table_name)
>>  }
>>
>>  /* Sets the column of 'row' in 'table'. */
>> -void
>> +static void
>>  set_column(const struct ctl_table_class *table,
>>             const struct ovsdb_idl_row *row, const char *arg,
>>             struct ovsdb_symbol_table *symtab)
>> @@ -2070,3 +2079,10 @@ set_column(const struct ctl_table_class *table,
>>      free(key_string);
>>      free(value_string);
>>  }
>
>
>
> I tried to separate private, public functions.  Do you also think it makes
> sense to move
> static function up to one of the private functions sections?
Yes. Will do.

Moving code around tends show a much bigger change set, and obscure
what is actually changed. I will
add a patch that just contain the function move.
>
>
>>
>> +
>> +void ctl_set_column(const char *table_name,
>> +                    const struct ovsdb_idl_row *row, const char *arg,
>> +                    struct ovsdb_symbol_table *symtab)
>> +{
>> +    set_column(get_table(table_name), row, arg, symtab);
>> +}
>> diff --git a/lib/db-ctl-base.h b/lib/db-ctl-base.h
>> index f14d27f..42b2e4a 100644
>> --- a/lib/db-ctl-base.h
>> +++ b/lib/db-ctl-base.h
>> @@ -245,14 +245,8 @@ struct ctl_table_class {
>>      struct ctl_row_id row_ids[2];
>>  };
>>
>> -/* Represents all tables in the schema.  User must define 'tables'
>> - * in implementation.  And the definition must end with an all-NULL
>> - * entry. */
>> -extern const struct ctl_table_class tables[];
>> -
>
>
>
> We should also delete this:
Good catch. Will do.
>
> diff --git a/lib/db-ctl-base.h b/lib/db-ctl-base.h
> index f14d27f..6089180 100644
> --- a/lib/db-ctl-base.h
> +++ b/lib/db-ctl-base.h
> @@ -44,7 +44,6 @@ struct table;
>   *   additional commands implemented by user.  (See 'struct ctl_context'
> for
>   *   more info)
>   *
> - * - the 'tables[]' for each table in the schema.
>   *
>  */
>
>
>
>>
>> -const struct ctl_table_class *get_table(const char *table_name);
>> -void set_column(const struct ctl_table_class *,
>> -                const struct ovsdb_idl_row *, const char *arg,
>> -                struct ovsdb_symbol_table *);
>> +void ctl_set_column(const char *table_name,
>> +                    const struct ovsdb_idl_row *, const char *arg,
>> +                    struct ovsdb_symbol_table *);
>>
>>  #endif /* db-ctl-base.h */
>> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
>> index c9af355..863bc73 100644
>> --- a/utilities/ovs-vsctl.c
>> +++ b/utilities/ovs-vsctl.c
>> @@ -1556,8 +1556,8 @@ add_port(struct ctl_context *ctx,
>>      }
>>
>>      for (i = 0; i < n_settings; i++) {
>> -        set_column(get_table("Port"), &port->header_, settings[i],
>> -                   ctx->symtab);
>> +        ctl_set_column("Port", &port->header_, settings[i],
>> +                       ctx->symtab);
>>      }
>>
>>      bridge_insert_port((bridge->parent ? bridge->parent->br_cfg
>> --
>> 1.9.1
>>
>
> Also, I think we should combine patch 2 and 3 since this patch will break
> the unittests.
>
They are separate changes. I will fix the unit tests.
>
> Thanks,
> Alex Wang,
>
>
>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>



More information about the dev mailing list