On 15 June 2015 at 19:35, Andy Zhou <[email protected]> wrote: > Add functions to support dealing with multiple schemas as a set. These
It seems that you are not using plural form of "schema" consistently (i.e. in the actual code you are using "schemata"). Take a look here[ http://english.stackexchange.com/questions/77764/plural-form-of-schema] regarding "schema" plural usage. I personally would prefer anglicized version because it is more obvious. Minor. Also, "dealing with multiple schemas" is loose description. Would you mind to be more specific in commit message? > functions will be useful in the following patch. > > Signed-off-by: Andy Zhou <[email protected]> > --- > ovsdb/ovsdb.c | 153 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ovsdb/ovsdb.h | 34 ++++++++++++- > 2 files changed, 186 insertions(+), 1 deletion(-) > > diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c > index 64355eb..5ec59ca 100644 > --- a/ovsdb/ovsdb.c > +++ b/ovsdb/ovsdb.c > @@ -455,3 +455,156 @@ ovsdb_remove_replica(struct ovsdb *db OVS_UNUSED, > struct ovsdb_replica *r) > list_remove(&r->node); > (r->class->destroy)(r); > } > + > Document this function's API. Here I am left wondering: 1) under what circumstances I would hit the ovs_assert() below. 2) what happens with schemap in case of error. 3) what does return value imply about error? +struct ovsdb_error * > +ovsdb_schemata_join(const struct shash *schemata, struct ovsdb_schema > **schemap) > I would rename schemap to dst_schema or joined_schema (especially if you will rename schemata to schemas). > +{ > + struct shash_node *first_node = shash_first(schemata), *node; > + struct ovsdb_schema *schema; > + struct ovsdb_error *error; > + > + ovs_assert(first_node); > + schema = ovsdb_schema_clone(first_node->data); > + > + SHASH_FOR_EACH (node, schemata) { > + struct ovsdb_schema *s = node->data; > + > + if (node != first_node) { > + error = ovsdb_schema_join(schema, s); > + > + if (error) { > + goto err; > + } > + } > + } > + > + *schemap = schema; > + return NULL; > + > +err: > + ovsdb_schema_destroy(schema); > + *schemap = NULL; > + return error; > +} > + > +void > +ovsdb_schemata_destroy(struct shash *schemata) > +{ > + struct shash_node *node, *next; > + > + SHASH_FOR_EACH_SAFE (node, next, schemata) { > + struct ovsdb_schema *schema = node->data; > + > + shash_delete(schemata, node); > + ovsdb_schema_destroy(schema); > + } > + shash_destroy(schemata); > +} > + > +struct ovsdb_error * > +ovsdb_schemata_from_files(struct sset *files, struct shash *schemata)' > Minor. How about "ovsdb_load_schemas[_from_files](...)" > +{ > + const char *filename; > + > + ovs_assert(schemata); > + ovs_assert(ovsdb_schemata_is_empty(schemata)); > Also, because of these asserts I would recommend you to document how this function is supposed to be used. I personally prefer to avoid asserts to "verify" function API usage, but I don't think there is agreement in CodyingStyle.md. + > + SSET_FOR_EACH(filename, files) { > + struct ovsdb_schema *schema; > + struct ovsdb_error *err; > + > + err = ovsdb_schema_from_file(filename, &schema); > + if (err) { > This function's API is lacking "symmetry" (this is an abstract term when creating something and deleting does not happen in a consistent way): 1. caller creates and provides shash 2. you free it in ovsdb_schemata_destroy() in case of error. If there really is a reason why this function should free shash then at least document that. + ovsdb_schemata_destroy(schemata); > + return err; > + } > + shash_add(schemata, schema->name, schema); > What happens if one passes duplicate file names in 'files'? > + } > + > + return NULL; > +} > + > +struct json * > +ovsdb_schemata_to_json(const struct shash *schemata) > +{ > + switch (shash_count(schemata)) { > + case 0: > + return NULL; > + > + case 1: > + { > + struct shash_node *node; > + struct ovsdb_schema *schema; > + > + node = shash_first(schemata); > + schema = node->data; > + > + return ovsdb_schema_to_json(schema); > + } > + > + default: > + { > + struct json *json_schemata; > + struct shash_node *node; > + > + json_schemata = json_array_create_empty(); > + SHASH_FOR_EACH (node, schemata) { > + struct ovsdb_schema *schema = node->data; > + json_array_add(json_schemata, > ovsdb_schema_to_json(schema)); > + } > + return json_schemata; > + } > + } > +} > + > +struct ovsdb_error * > +ovsdb_schemata_from_json(struct json *json, struct shash *schemata) > +{ > + struct ovsdb_schema *schema; > + struct ovsdb_error *err; > + > + ovs_assert(ovsdb_schemata_is_empty(schemata)); > + > + switch (json->type) { > + case JSON_OBJECT: > + err = ovsdb_schema_from_json(json, &schema); > + if (err) { > + goto error; > + } > + shash_add(schemata, schema->name, schema); > + break; > + > + case JSON_ARRAY: > + { > + struct json_array *array = json_array(json); > + size_t i; > + > + for(i = 0; i < array->n; i++) { > + err = ovsdb_schema_from_json(array->elems[i], &schema); > + > + if (err) { > + goto error; > + } > + shash_add(schemata, schema->name, schema); > + } > + } > + break; > + > + case JSON_NULL: > + case JSON_FALSE: > + case JSON_TRUE: > + case JSON_INTEGER: > + case JSON_REAL: > + case JSON_STRING: > + case JSON_N_TYPES: > + default: > + OVS_NOT_REACHED(); > + } > + > + return NULL; > + > +error: > + ovsdb_schemata_destroy(schemata); > Same here, caller only by looking into err can find out whether this function released shash in ovsdb_schemata_destroy(). > + > + return err; > +} > diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h > index 215e046..e3f3b1e 100644 > --- a/ovsdb/ovsdb.h > +++ b/ovsdb/ovsdb.h > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -51,6 +51,38 @@ struct ovsdb_error *ovsdb_schema_from_json(struct json > *, > OVS_WARN_UNUSED_RESULT; > struct json *ovsdb_schema_to_json(const struct ovsdb_schema *); > > +/* Multiple schemas. */ > +struct ovsdb_error *ovsdb_schemata_from_files(struct sset *files, > + struct shash* schemata); > +void ovsdb_schemata_destroy(struct shash *schemata); > +struct ovsdb_error * ovsdb_schemata_from_files(struct sset *files, > + struct shash *schemata) > + OVS_WARN_UNUSED_RESULT; > +struct json *ovsdb_schemata_to_json(const struct shash *schmata); > +struct ovsdb_error * ovsdb_schemata_from_json(struct json *json, > + struct shash *schemata) > + OVS_WARN_UNUSED_RESULT; > +struct ovsdb_error *ovsdb_schemata_join(const struct shash *schemata, > + struct ovsdb_schema **schema) > + OVS_WARN_UNUSED_RESULT; > + > +static inline bool > +ovsdb_schemata_is_empty(const struct shash *schemata) > +{ > + return (!shash_count(schemata)); > +} > + > +static inline bool > +ovsdb_schemata_is_null_or_empty(const struct shash *schemata) > +{ > + return (!schemata || ovsdb_schemata_is_empty(schemata)); > +} > + > +static inline bool > +ovsdb_schemata_is_non_empty(const struct shash *schemata) > +{ > + return (schemata && !ovsdb_schemata_is_empty(schemata)); > +} > > bool ovsdb_schema_equal(const struct ovsdb_schema *, > const struct ovsdb_schema *); > -- > 1.9.1 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
