On Wed, Mar 09, 2016 at 12:03:54AM +0000, Rodriguez Betancourt, Esteban wrote:
> In the C IDL, allows to create multicolumn indexes in the
> tables, that are keep synched with the data in the replica.
>
> Signed-off-by: Esteban Rodriguez Betancourt <esteb...@hpe.com>
Until now, ovsdb-idl.h has been divided into sections separated by
page-breaks (control-L), and each section has had a comment explaining
basically what that section describes. Some of the sections, like the
one for transactions, has a lot of detail. This commit adds a new
section, but it doesn't add a page break or an explanatory comment. I
think it should do both. The design document from patch 1 might be a
good place from which to draw for the comment.
This patch and patch 2 has a couple of typedefs for function pointers.
CodingStyle.md says:
A function type is a good use for a typedef because it can clarify
code. The type should be a function type, not a pointer-to-function
type. That way, the typedef name can be used to declare function
prototypes. (It cannot be used for function definitions, because that
is explicitly prohibited by C89 and C99.)
Do you think that the sorting order in struct ovsdb_idl_index is useful,
that is, do you see a common use for indexes in descending order? If
indexes in descending order are only rarely useful, then they could be
implemented through a custom comparison function.
I don't understand, in ovsdb_idl_index_compare(), why NULL != NULL. It
seems to me that this is risky and likely to cause confusion, if not now
then later.
I don't understand why indexes have string names. It seems like kind of
a curious design. Why isn't the client expected to retain a pointer to
the index that it wants to traverse, instead of a name?
Please use xmalloc() instead of malloc().
I found myself wondering whether there's value in doing dynamic
allocation of the three parallel arrays inside struct ovsdb_idl_index.
It would be very unusual to have an index over more than, say, 3
columns, and so it might be easier to have fixed-size arrays of 3
elements (or a few more, to allow room for expansion). I imagine that
the columns in indexes will be fixed at build time, after all.
ovsdb_idl_index_add_column() seems to be working hard to avoid using
xrealloc(), but I don't know why.
I don't understand why only certain columns have a default comparison
function. Why not use ovsdb_datum_compare_3way() as the default for
every column? I guess that the ovsdb_datums and ovsdb_type would have
to be passed into column_comparator instead of a pair of void *
pointers, but that would arguably be an improvement anyhow, certainly
from a type-safety viewpoint.
Do you have an example of a useful use for a custom comparison function?
Did you consider defining a struct of three members instead of using
three parallel arrays in struct ovsdb_idl_index? It might well be
cleaner, and certainly would be for memory allocation issues.
It looks like the clients don't actually need to see the definition of
struct ovsdb_idl_index, so perhaps it should be moved into
ovsdb-idl-provider.h.
This patch has some minor coding style issues; I suggest reading
CodingStyle.md.
The generic comparison functions takes 'row_sync' into account. Do
custom comparison functions also need to take 'row_sync' into account?
(Why or why not?)
It looks like row_sync is needed because the skiplist data structure
implemented in the previous patch does not tolerate insertion of
duplicates. That seems like a serious drawback for a data structure
that is supposed to be used as an index that may contain duplicates.
Did you consider implementing a skip list (or other balanced tree
structure) that gracefully supports duplicates? That would be a much
more straightforward solution to the problem.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
