[ovs-dev] [ovsdb join 2/5] ovsdb: add supporting functions for schemata
Ansis Atteka
ansisatteka at gmail.com
Sun Jun 21 20:29:39 UTC 2015
On 15 June 2015 at 19:35, Andy Zhou <azhou at 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.
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 <azhou at 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:
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
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list