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. It seems like a build
assertion of some sort (maybe that the size of "ovsdb_row" is the same as the
offset of "fields" plus the length of "fields?) would be good to prevent subtle
errors in this.
> +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"?
> @@ -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 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...)
Looks good.
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
