On 22 June 2015 at 13:31, Ansis Atteka <[email protected]> wrote:
> > > On 22 June 2015 at 13:00, Andy Zhou <[email protected]> wrote: > >> On Sun, Jun 21, 2015 at 1:29 PM, Ansis Atteka <[email protected]> >> wrote: >> > >> > >> > 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. >> > >> I treat them as interchangeable terms. Sure I can switch to schemas if >> it is easier to >> read. > > >> > Minor. Also, "dealing with multiple schemas" is loose description. >> Would you >> > mind to be more specific in commit message? >> > >> Yes, I will add more context. >> >> >> >> 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: >> >> How about adding the following comment? >> >> +/* Join all schemas contains within 'schemas' to produce a single >> joined schema. >> > There is a typo above. How about: > > This function joins all OVSDB schemas in 'schemas' into 'joined_schema'. > If 'schemas' is empty then this function's behavior is undefined. > > > + * 'schmas' passed in is expected to contain at least one schema. >> + * >> + * On success, this function returns NULL, 'schemap' points to the >> newly created >> + * joined schema, The caller is responsible for reclaim its memory by >> calling >> + * ovsdb_schema_destroy(). >> > + * >> + * On error, an ovsdb_error will be allocated describing the error, the >> caller >> + * is responsible for dispose its memory. */ > > >> > 1) under what circumstances I would hit the ovs_assert() below. >> >> If 'schmas' is empty, this is not expected by the function. >> > > Those 3 questions that I asked here weren't meant to be answered by you as > part of review in email. :) > > I simply posted them as example questions that came in my mind (and > probably everyone's else) while looking into this code. Basically > function's comment should define valid input domain for arguments. If you > are using asserts() on input arguments then this means that your function > does not expect certain values and these certain values should be > documented i functions comment because they would leave to undefined > behavior. > had a typo above: s/i functions comment because they would leave/in function's comment because they would lead to undefined behavior. Also, *I am not saying* that you have to remove ovs_assert() in your code, if you specify in function's comment that behavior is undefined for certain input values. Anyway, this is a really minor thing and we should not spend too much time. > >> >> > 2) what happens with schemap in case of error. >> *schemap will be set to NULL. >> > 3) what does return value imply about error? >> What do you mean? It is of type ovsdb_error which is standard. >> > > With 3) I meant that it is a good practice to document how functions > convey error to its caller. You already did that by adding comment "... On > error, an ovsdb_error will be allocated describing the error, the caller is > responsible for dispose its memory. ". > > > > >> >> +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). >> > >> Make sense. Will do. >> > >> > >> >> >> >> +{ >> >> + 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](...)" >> >> >> Sounds good, for consistency with functions around it, I will probably >> use ovsdb_schmeas_load() >> >> +{ >> >> + 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. >> To address other comments you have, I will change the API to use a >> double pointer, as >> struct shash **schemas. Sure, will add comments to this function. >> > >> > I personally prefer to avoid asserts to "verify" function API usage, >> but I >> > don't think there is agreement in CodyingStyle.md. >> I can remove the asserts with the API change. >> > >> >> + >> >> + 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. >> > >> The API change should address this -- it will only free the memory >> allocated within >> this function. >> >> > 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'? >> sset_add will remove that duplication. >> >> We probably should not allow schema name duplication either. I will >> use shash_add_once >> in stead shash_add here. >> >> >> >> >> + } >> >> + >> >> + 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(). >> >> >> Will apply the same API change to this functions as well. >> >> + >> >> + 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
