On Mon, Jun 06, 2011 at 12:39:13AM -0700, Justin Pettit wrote:
> On Jun 2, 2011, at 4:19 PM, Ben Pfaff wrote:
>
> > @@ -57,6 +57,9 @@ ovsdb_table_schema_create(const char *name, bool mutable,
> >
> > + ts->n_indexes = 0;
> > + ts->indexes = NULL;
>
> This isn't strictly necessary, since "ts" is allocated with
> xzalloc(). The function should be seldom called, so maybe it's
> worth keeping for clarity...
>
> > +/* Returns the offset in bytes from the start of an ovsdb_row for 'table'
> > to
> > + * the hmap_node for the index numbered 'i'. */
> > +static size_t
> > +ovsdb_row_index_offset__(const struct ovsdb_table *table, size_t i)
> > +{
> > + size_t n_fields = shash_count(&table->schema->columns);
> > + return (offsetof(struct ovsdb_row, fields)
> > + + n_fields * sizeof(struct ovsdb_datum)
> > + + i * sizeof(struct hmap_node));
> > +}
>
> This seems very fragile. If someone changes the definition of
> "ovsdb_row" to have some data after "fields", then this will break.
"fields" is a "flexible array member". You can't put a member after a
flexible array member:
../ovsdb/row.h:61:24: error: flexible array member not at end of struct
> > +def column_set_from_json(json, columns):
> > ....
> > + elif column_name not in columns:
> > + raise error.Error("%s is not a valid column name"
> > + % column_name, column_name)
>
> Shouldn't this second argument be "json"?
Yes, thanks.
> > @@ -150,6 +169,7 @@ class TableSchema(object):
> > mutable = parser.get_optional("mutable", [bool], True)
> > max_rows = parser.get_optional("maxRows", [int])
> > is_root = parser.get_optional("isRoot", [bool], False)
> > + indexes_json = parser.get_optional("indexes", [list], [])
>
> The naming of these variables is pretty inconsistent. At the very
> least, I'd have "columnsJson" and "index_json" use the same
> convention.
This code started out written in an ad-hoc Python style, and then as I
added code I started using the Google style guide. I didn't convert
existing code, though, so there's a mix.
I added a commit to change this function to use underscores
consistently.
> (This is not for this series, but it may be possible to simplify the
> "ovs-vsctl list" command if we use the unique index instead of
> having to specify the "name" field. Just a thought, anyway...)
Can you explain further?
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
