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.
+ * '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.
> 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.
>
>> +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
