On 22 June 2015 at 13:00, Andy Zhou <az...@nicira.com> wrote:
> On Sun, Jun 21, 2015 at 1:29 PM, Ansis Atteka <ansisatt...@gmail.com>
> wrote:
> >
> >
> > On 15 June 2015 at 19:35, Andy Zhou <az...@nicira.com> 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 <az...@nicira.com>
> >> ---
> >> 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.
>
> > 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
> >> dev@openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
