On Wed, Jul 8, 2015 at 11:44 PM, Alex Wang <al...@nicira.com> wrote:
>
>
> On Tue, Jul 7, 2015 at 8:08 PM, Andy Zhou <az...@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 <az...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
