[ovs-dev] [PATCH 6/7] Add fixes, improvements, refactors and cleans code

Lutz, Arnoldo arnoldo.lutz.guevara at hpe.com
Mon Feb 22 19:59:10 UTC 2016


Added delta calculations for PMUs

Added additional checks to make sure that only one update operation is
performed on each key for each transaction. If several update operations are
issued by the client, these are resolved and only one is applied on commit.

Delayed operation validity checks until commit, because doing these validations
when an operation is added could lead to inconsistent results when multiple
operations are performed on the same key.

Improves JSON generation for PMUs

Improves JSON generation by grouping together all delete mutations into one
single mutation for each row, which shortens overall JSON message length. A
similar improvement is applied to insert mutations.

Code was cleaned up as well (deleted dead code, commented complex functions).

Add code to test for partial update map column

Insert basic funtionality for testing partial update column
and add a new test table named "simple2"
Tests running ok
Insert the code for testing this feature.
Add extra conditions to testing partial updates.
Clean compilation warnings.
Clean code

Added warning when trying to delete a missing key

Set txn_sdd_pmu() as static

Function ovsdb_idl_txn_add_pmu() will only be used inside this file, therefore
it ts better to make it static.

Re-factor code to make txn_commit() cleaner.

Change pmu structure name to map_op

The name 'pmu' doesn't match the coding guidelines for OVS. It is changed to
'map_op' which is more descriptive of the structure function (which is to
perform a partial update operation on a map). Other structure, function and
file names were changed accordingly.

Signed-off-by: Edward Aymerich <edward.aymerich at hpe.com>
---
 lib/automake.mk          |   4 +-
 lib/ovsdb-idl-provider.h |   8 +-
 lib/ovsdb-idl.c          | 305 +++++++++++++++++++++++++++++------------------
 lib/ovsdb-map-op.c       | 171 ++++++++++++++++++++++++++
 lib/ovsdb-map-op.h       |  45 +++++++
 lib/ovsdb-pmu.c          | 117 ------------------
 lib/ovsdb-pmu.h          |  44 -------
 ovsdb/ovsdb-idlc.in      |   8 +-
 tests/idltest.ovsschema  |  24 +++-
 tests/idltest2.ovsschema |  24 +++-
 tests/ovsdb-idl.at       |  34 ++++++
 tests/test-ovsdb.c       | 104 ++++++++++++++++
 12 files changed, 596 insertions(+), 292 deletions(-)
 create mode 100644 lib/ovsdb-map-op.c
 create mode 100644 lib/ovsdb-map-op.h
 delete mode 100644 lib/ovsdb-pmu.c
 delete mode 100644 lib/ovsdb-pmu.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 770ce72..f8e1c43 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -185,10 +185,10 @@ lib_libopenvswitch_la_SOURCES = \
 	lib/ovsdb-idl-provider.h \
 	lib/ovsdb-idl.c \
 	lib/ovsdb-idl.h \
+	lib/ovsdb-map-op.c \
+	lib/ovsdb-map-op.h \
 	lib/ovsdb-parser.c \
 	lib/ovsdb-parser.h \
-	lib/ovsdb-pmu.c \
-	lib/ovsdb-pmu.h \
 	lib/ovsdb-types.c \
 	lib/ovsdb-types.h \
 	lib/packets.c \
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 0851c11..f265dcd 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -20,7 +20,7 @@
 #include "list.h"
 #include "ovsdb-idl.h"
 #include "ovsdb-types.h"
-#include "ovsdb-pmu.h"
+#include "ovsdb-map-op.h"
 #include "shash.h"
 #include "uuid.h"
 
@@ -37,14 +37,14 @@ struct ovsdb_idl_row {
     unsigned long int *prereqs; /* Bitmap of columns to verify in "old". */
     unsigned long int *written; /* Bitmap of columns from "new" to write. */
     struct hmap_node txn_node;  /* Node in ovsdb_idl_txn's list. */
+    unsigned long int *map_op_written; /* Bitmap of columns with pending map
+                                        * operations. */
+    struct map_op_list **map_op_lists; /* List of lists of map operations. */
 
     /* Tracking data */
     unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX];
     struct ovs_list track_node; /* Rows modified/added/deleted by IDL */
     unsigned long int *updated; /* Bitmap of columns updated by IDL */
-
-    unsigned long int *partial_map_written; /* Bitmap of columns containing partial maps */
-    struct pmul **partial_map_lists; /* List of lists of partial updates. */
 };
 
 struct ovsdb_idl_column {
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index ff5e26a..ae09c5c 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -190,6 +190,12 @@ static void ovsdb_idl_row_clear_arcs(struct ovsdb_idl_row *, bool destroy_dsts);
 static void ovsdb_idl_txn_abort_all(struct ovsdb_idl *);
 static bool ovsdb_idl_txn_process_reply(struct ovsdb_idl *,
                                         const struct jsonrpc_msg *msg);
+static bool ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *,
+                                            struct json *);
+static void ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *,
+                                     const struct ovsdb_idl_column *,
+                                     struct ovsdb_datum *,
+                                     enum map_op_type);
 
 static void ovsdb_idl_send_lock_request(struct ovsdb_idl *);
 static void ovsdb_idl_send_unlock_request(struct ovsdb_idl *);
@@ -203,10 +209,7 @@ ovsdb_idl_table_from_class(const struct ovsdb_idl *,
                            const struct ovsdb_idl_table_class *);
 static bool ovsdb_idl_track_is_set(struct ovsdb_idl_table *table);
 
-void ovsdb_idl_txn_add_pmu(struct ovsdb_idl_row *,
-                           const struct ovsdb_idl_column *,
-                           struct ovsdb_datum *,
-                           enum pmu_operation);
+
 
 /* Creates and returns a connection to database 'remote', which should be in a
  * form acceptable to jsonrpc_session_open().  The connection will maintain an
@@ -1562,28 +1565,28 @@ ovsdb_idl_row_create(struct ovsdb_idl_table *table, const struct uuid *uuid)
     hmap_insert(&table->rows, &row->hmap_node, uuid_hash(uuid));
     row->uuid = *uuid;
     row->table = table;
-    row->partial_map_written = NULL;
-    row->partial_map_lists = NULL;
+    row->map_op_written = NULL;
+    row->map_op_lists = NULL;
     return row;
 }
 
 static void
-ovsdb_idl_destroy_all_pmuls(struct ovsdb_idl_row *row){
-    if (row->partial_map_written) {
-        /* Clear PMULs */
+ovsdb_idl_destroy_all_map_op_lists(struct ovsdb_idl_row *row){
+    if (row->map_op_written) {
+        /* Clear Map Operation Lists */
         size_t idx, n_columns;
         const struct ovsdb_idl_column *columns;
         const struct ovsdb_type *type;
         n_columns = row->table->class->n_columns;
         columns = row->table->class->columns;
-        BITMAP_FOR_EACH_1 (idx, n_columns, row->partial_map_written) {
+        BITMAP_FOR_EACH_1 (idx, n_columns, row->map_op_written) {
             type = &columns[idx].type;
-            pmul_destroy(row->partial_map_lists[idx], type);
+            map_op_list_destroy(row->map_op_lists[idx], type);
         }
-        free(row->partial_map_lists);
-        bitmap_free(row->partial_map_written);
-        row->partial_map_lists = NULL;
-        row->partial_map_written = NULL;
+        free(row->map_op_lists);
+        bitmap_free(row->map_op_written);
+        row->map_op_lists = NULL;
+        row->map_op_written = NULL;
     }
 }
 
@@ -1601,7 +1604,7 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
         if (list_is_empty(&row->track_node)) {
             list_push_front(&row->table->track_list, &row->track_node);
         }
-        ovsdb_idl_destroy_all_pmuls(row);
+        ovsdb_idl_destroy_all_map_op_lists(row);
     }
 }
 
@@ -2132,7 +2135,7 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
     txn->idl->txn = NULL;
 
     HMAP_FOR_EACH_SAFE (row, next, txn_node, &txn->txn_rows) {
-        ovsdb_idl_destroy_all_pmuls(row);
+        ovsdb_idl_destroy_all_map_op_lists(row);
         if (row->old) {
             if (row->written) {
                 ovsdb_idl_row_unparse(row);
@@ -2161,6 +2164,115 @@ ovsdb_idl_txn_disassemble(struct ovsdb_idl_txn *txn)
     hmap_init(&txn->txn_rows);
 }
 
+static bool
+ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *row,
+                                struct json *mutations)
+{
+    const struct ovsdb_idl_table_class *class = row->table->class;
+    size_t idx;
+    bool any_mutations = false;
+
+    BITMAP_FOR_EACH_1(idx, class->n_columns, row->map_op_written) {
+        struct map_op_list *map_op_list;
+        const struct ovsdb_idl_column *column;
+        enum ovsdb_atomic_type key_type, value_type;
+        struct json *mutation, *map, *col_name, *mutator;
+        struct json *del_set, *ins_map;
+        bool any_del, any_ins;
+
+        map_op_list = row->map_op_lists[idx];
+        column = &class->columns[idx];
+        key_type = column->type.key.type;
+        value_type = column->type.value.type;
+
+        del_set = json_array_create_empty();
+        ins_map = json_array_create_empty();
+        any_del = false;
+        any_ins = false;
+
+        for (struct map_op *map_op = map_op_list_first(map_op_list); map_op;
+             map_op = map_op_list_next(map_op_list, map_op)) {
+
+            if (map_op_type(map_op) == MAP_OP_UPDATE) {
+                /* Find out if value really changed */
+                struct ovsdb_datum *old_datum, *new_datum;
+                unsigned int pos;
+                old_datum = &row->old[idx];
+                new_datum = map_op_datum(map_op);
+                pos = ovsdb_datum_find_key(old_datum,
+                                           &new_datum->keys[0],
+                                           key_type);
+                if (ovsdb_atom_equals(&new_datum->values[0],
+                                      &old_datum->values[pos],
+                                      value_type)) {
+                    /* No change in value. Move on to next update. */
+                    continue;
+                }
+            } else if (map_op_type(map_op) == MAP_OP_DELETE){
+                /* Verify that there is a key to delete */
+                unsigned int pos;
+                pos = ovsdb_datum_find_key(&row->old[idx],
+                                           &map_op_datum(map_op)->keys[0],
+                                           key_type);
+                if (pos == UINT_MAX) {
+                    /* No key to delete. Move on to next update. */
+                    VLOG_WARN("Trying to delete a key that doesn't "
+                              "exist in the map.");
+                    continue;
+                }
+            }
+
+            if (map_op_type(map_op) == MAP_OP_INSERT) {
+                map = json_array_create_2(
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->keys[0],
+                                       key_type),
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->values[0],
+                                       value_type));
+                json_array_add(ins_map, map);
+                any_ins = true;
+            } else { /* MAP_OP_UPDATE or MAP_OP_DELETE */
+                map = ovsdb_atom_to_json(&map_op_datum(map_op)->keys[0],
+                                         key_type);
+                json_array_add(del_set, map);
+                any_del = true;
+            }
+
+            /* Generates an additional insert mutate for updates */
+            if (map_op_type(map_op) == MAP_OP_UPDATE) {
+                map = json_array_create_2(
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->keys[0],
+                                       key_type),
+                    ovsdb_atom_to_json(&map_op_datum(map_op)->values[0],
+                                       value_type));
+                json_array_add(ins_map, map);
+                any_ins = true;
+            }
+        }
+
+        if (any_del) {
+            col_name = json_string_create(column->name);
+            mutator = json_string_create("delete");
+            map = json_array_create_2(json_string_create("set"), del_set);
+            mutation = json_array_create_3(col_name, mutator, map);
+            json_array_add(mutations, mutation);
+            any_mutations = true;
+        } else {
+            json_destroy(del_set);
+        }
+        if (any_ins) {
+            col_name = json_string_create(column->name);
+            mutator = json_string_create("insert");
+            map = json_array_create_2(json_string_create("map"), ins_map);
+            mutation = json_array_create_3(col_name, mutator, map);
+            json_array_add(mutations, mutation);
+            any_mutations = true;
+        } else {
+            json_destroy(ins_map);
+        }
+    }
+    return any_mutations;
+}
+
 /* Attempts to commit 'txn'.  Returns the status of the commit operation, one
  * of the following TXN_* constants:
  *
@@ -2348,53 +2460,25 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
             }
         }
 
-        /* Add Partial Map Updates (mutate ops) */
-        if (row->partial_map_written) {
+        /* Add Map Operation (Partial Map Updates). */
+        if (row->map_op_written) {
             struct json *op, *mutations;
-            size_t idx;
+            bool any_mutations;
 
             op = json_object_create();
             json_object_put_string(op, "op", "mutate");
             json_object_put_string(op, "table", class->name);
             json_object_put(op, "where", where_uuid_equals(&row->uuid));
             mutations = json_array_create_empty();
+            any_mutations = ovsdb_idl_txn_extract_mutations(row, mutations);
+            json_object_put(op, "mutations", mutations);
 
-            BITMAP_FOR_EACH_1(idx, class->n_columns,
-                              row->partial_map_written) {
-                struct pmul *pmul = row->partial_map_lists[idx];
-                const struct ovsdb_idl_column *column = &class->columns[idx];
-                enum ovsdb_atomic_type key_type = column->type.key.type;
-                struct json *mutation, *map, *col_name, *mutator;
-
-                for (struct pmu *pmu = pmul_first(pmul); pmu;
-                     pmu = pmul_next(pmul, pmu)) {
-                    col_name = json_string_create(column->name);
-                    if (pmu_operation(pmu) == PMU_INSERT) {
-                        mutator = json_string_create("insert");
-                        map = ovsdb_datum_to_json(pmu_datum(pmu),
-                                                  &column->type);
-                    } else { /* PMU_UPDATE or PMU_DELETE */
-                        mutator = json_string_create("delete");
-                        map = ovsdb_atom_to_json(&pmu_datum(pmu)->keys[0],
-                                                 key_type);
-                    }
-                    mutation = json_array_create_3(col_name, mutator, map);
-                    json_array_add(mutations, mutation);
-
-                    /* Generates an additional mutate for updates */
-                    if (pmu_operation(pmu) == PMU_UPDATE) {
-                        col_name = json_string_create(column->name);
-                        mutator = json_string_create("insert");
-                        map = ovsdb_datum_to_json(pmu_datum(pmu),
-                                                  &column->type);
-                        mutation = json_array_create_3(col_name, mutator, map);
-                        json_array_add(mutations, mutation);
-                    }
-                }
+            if (any_mutations) {
+                json_array_add(operations, op);
+                any_updates = true;
+            } else {
+                json_destroy(op);
             }
-            json_object_put(op, "mutations", mutations);
-            json_array_add(operations, op);
-            any_updates = true;
         }
     }
 
@@ -3276,39 +3360,34 @@ ovsdb_idl_loop_commit_and_wait(struct ovsdb_idl_loop *loop)
     ovsdb_idl_wait(loop->idl);
 }
 
-/* Skeleton functions needed to handle partial for map_columns
- * This functions must be moved to a better place when finished implementation
- *
- */
-
-/* Creates a new Partial Map Update into current transaction */
-void
-ovsdb_idl_txn_add_pmu(struct ovsdb_idl_row *row,
-                      const struct ovsdb_idl_column *column,
-                      struct ovsdb_datum *datum,
-                      enum pmu_operation operation)
+/* Inserts a new Map Operation into current transaction. */
+static void
+ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *row,
+                         const struct ovsdb_idl_column *column,
+                         struct ovsdb_datum *datum,
+                         enum map_op_type op_type)
 {
     const struct ovsdb_idl_table_class *class;
     size_t column_idx;
-    struct pmu *pmu;
+    struct map_op *map_op;
 
     class = row->table->class;
     column_idx = column - class->columns;
 
-    /* Check if a PMU list exist for this column */
-    if(!row->partial_map_written){
-        row->partial_map_written = bitmap_allocate(class->n_columns);
-        row->partial_map_lists = xzalloc(class->n_columns *
-                                         sizeof *row->partial_map_lists);
+    /* Check if a Map Operation List exist for this column */
+    if (!row->map_op_written) {
+        row->map_op_written = bitmap_allocate(class->n_columns);
+        row->map_op_lists = xzalloc(class->n_columns *
+                                    sizeof *row->map_op_lists);
     }
-    if(!row->partial_map_lists[column_idx]) {
-        row->partial_map_lists[column_idx] = pmul_create();
+    if (!row->map_op_lists[column_idx]) {
+        row->map_op_lists[column_idx] = map_op_list_create();
     }
 
-    /* Add PMU to corresponding list */
-    pmu = pmu_create(datum, operation);
-    bitmap_set1(row->partial_map_written, column_idx);
-    pmul_add_pmu(row->partial_map_lists[column_idx], pmu);
+    /* Add a Map Operation to the corresponding list */
+    map_op = map_op_create(datum, op_type);
+    bitmap_set1(row->map_op_written, column_idx);
+    map_op_list_add(row->map_op_lists[column_idx], map_op, &column->type);
 
     /* Add this row to transaction's list of rows */
     if (hmap_node_is_null(&row->txn_node)) {
@@ -3317,24 +3396,21 @@ ovsdb_idl_txn_add_pmu(struct ovsdb_idl_row *row,
     }
 }
 
-/*
-static unsigned char *
-ovsdb_idl_get_row_column_mode(const struct ovsdb_idl_row *row,
-                              unsigned int column_idx)
-{
-    unsigned int column_idx = column - row->table->class->columns;
-    return row->table->class->modes[column_idx];
-}*/
-
-/* Takes ownership of datum */
+/* Inserts the key-value specified in 'datum' into the map in 'column' in
+ * 'row_'. If the key already exist in 'column', then it's value is updated
+ * with the value in 'datum'. The key-value in 'datum' must be of the same type
+ * as the keys-values in 'column'. This function takes ownership of 'datum'.
+ *
+ * Usually this function is used indirectly through one of the "update"
+ * functions generated by vswitch-idl. */
 void
 ovsdb_idl_txn_write_partial_map(const struct ovsdb_idl_row *row_,
                                 const struct ovsdb_idl_column *column,
                                 struct ovsdb_datum *datum)
 {
     struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
-    enum ovsdb_atomic_type key_type, value_type;
-    enum pmu_operation operation;
+    enum ovsdb_atomic_type key_type;
+    enum map_op_type op_type;
     unsigned int column_idx, pos;
     struct ovsdb_datum *old_datum;
 
@@ -3360,57 +3436,48 @@ ovsdb_idl_txn_write_partial_map(const struct ovsdb_idl_row *row_,
     pos = ovsdb_datum_find_key(old_datum, &datum->keys[0], key_type);
     if (pos == UINT_MAX) {
         /* Insert operation */
-        operation = PMU_INSERT;
+        op_type = MAP_OP_INSERT;
     } else {
         /* Update operation */
-        operation = PMU_UPDATE;
-        value_type = column->type.value.type;
-        if (ovsdb_atom_equals(&datum->values[0], &old_datum->values[pos],
-                              value_type)) {
-            /* Same value as before. Nothing to do, except destroy datum. */
-            ovsdb_datum_destroy(datum, &column->type);
-            return;
-        }
+        op_type = MAP_OP_UPDATE;
     }
-    ovsdb_idl_txn_add_pmu(row, column, datum, operation);
+    ovsdb_idl_txn_add_map_op(row, column, datum, op_type);
 }
 
-/* Takes ownership of datum */
+/* Deletes the key specified in 'datum' from the map in 'column' in 'row_'.
+ * The key in 'datum' must be of the same type as the keys in 'column'.
+ * The value in 'datum' must be NULL. This function takes ownership of
+ * 'datum'.
+ *
+ * Usually this function is used indirectly through one of the "update"
+ * functions generated by vswitch-idl. */
 void
 ovsdb_idl_txn_delete_partial_map(const struct ovsdb_idl_row *row_,
-                                const struct ovsdb_idl_column *column,
-                                struct ovsdb_datum *datum)
+                                 const struct ovsdb_idl_column *column,
+                                 struct ovsdb_datum *datum)
 {
     struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
-    enum ovsdb_atomic_type key_type;
-    unsigned int column_idx, pos;
+    unsigned int column_idx;
 
     /* Verify that this column is being monitored */
     column_idx = column - row->table->class->columns;
     if (!(row->table->modes[column_idx] & OVSDB_IDL_MONITOR)) {
         VLOG_WARN("ovsdb_idl_txn_delete_partial_map(): Trying to update a non"
                   "-monitored column.");
-        ovsdb_datum_destroy(datum, &column->type);
+        struct ovsdb_type type_ = column->type;
+        type_.value.type = OVSDB_TYPE_VOID;
+        ovsdb_datum_destroy(datum, &type_);
         return;
     }
 
     if (datum->n != 1) {
         VLOG_WARN("ovsdb_idl_txn_delete_partial_map(): Trying to delete using"
                   " an invalid datum.");
-        ovsdb_datum_destroy(datum, &column->type);
+        struct ovsdb_type type_ = column->type;
+        type_.value.type = OVSDB_TYPE_VOID;
+        ovsdb_datum_destroy(datum, &type_);
         return;
     }
 
-    /* Find out if there exist a key to delete */
-    key_type = column->type.key.type;
-    pos = ovsdb_datum_find_key(&row->old[column_idx], &datum->keys[0],
-                               key_type);
-    if (pos == UINT_MAX) {
-        /* Nothing to delete. Nothing to do, except destroy datum. */
-        struct ovsdb_type type = column->type;
-        type.value.type = OVSDB_TYPE_VOID;
-        ovsdb_datum_destroy(datum, &type);
-        return;
-    }
-    ovsdb_idl_txn_add_pmu(row, column, datum, PMU_DELETE);
+    ovsdb_idl_txn_add_map_op(row, column, datum, MAP_OP_DELETE);
 }
diff --git a/lib/ovsdb-map-op.c b/lib/ovsdb-map-op.c
new file mode 100644
index 0000000..6d39122
--- /dev/null
+++ b/lib/ovsdb-map-op.c
@@ -0,0 +1,171 @@
+/* Copyright (C) 2016 Hewlett Packard Enterprise Development LP
+�* All Rights Reserved.
+�*
+�* Licensed under the Apache License, Version 2.0 (the "License"); you may
+�* not use this file except in compliance with the License. You may obtain
+�* a copy of the License at
+�*
+�*�����http://www.apache.org/licenses/LICENSE-2.0
+�*
+�* Unless required by applicable law or agreed to in writing, software
+�* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+�* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+�* License for the specific language governing permissions and limitations
+�* under the License.
+�*/
+
+#include <config.h>
+#include "ovsdb-map-op.h"
+#include "util.h"
+#include "hmap.h"
+#include "hash.h"
+
+/* Map Operation: a Partial Map Update */
+struct map_op {
+    struct hmap_node node;
+    struct ovsdb_datum *datum;
+    enum map_op_type type;
+};
+
+/* List of Map Operations */
+struct map_op_list {
+    struct hmap hmap;
+};
+
+static void map_op_destroy_datum(struct map_op *, const struct ovsdb_type *);
+static struct map_op* map_op_list_find(struct map_op_list *, struct map_op *,
+                                       const struct ovsdb_type *, size_t);
+
+struct map_op*
+map_op_create(struct ovsdb_datum *datum, enum map_op_type type)
+{
+    struct map_op *map_op = xmalloc(sizeof *map_op);
+    map_op->node.hash = 0;
+    map_op->node.next = HMAP_NODE_NULL;
+    map_op->datum = datum;
+    map_op->type = type;
+    return map_op;
+}
+
+static void
+map_op_destroy_datum(struct map_op *map_op, const struct ovsdb_type *type)
+{
+    if (map_op->type == MAP_OP_DELETE){
+        struct ovsdb_type type_ = *type;
+        type_.value.type = OVSDB_TYPE_VOID;
+        ovsdb_datum_destroy(map_op->datum, &type_);
+    } else {
+        ovsdb_datum_destroy(map_op->datum, type);
+    }
+    map_op->datum = NULL;
+}
+
+void
+map_op_destroy(struct map_op *map_op, const struct ovsdb_type *type)
+{
+    map_op_destroy_datum(map_op, type);
+    free(map_op);
+}
+
+struct ovsdb_datum*
+map_op_datum(const struct map_op *map_op)
+{
+    return map_op->datum;
+}
+
+enum map_op_type
+map_op_type(const struct map_op *map_op)
+{
+    return map_op->type;
+}
+
+struct map_op_list*
+map_op_list_create()
+{
+    struct map_op_list *list = xmalloc(sizeof *list);
+    hmap_init(&list->hmap);
+    return list;
+}
+
+void
+map_op_list_destroy(struct map_op_list *list, const struct ovsdb_type *type)
+{
+    struct map_op *map_op, *next;
+    HMAP_FOR_EACH_SAFE (map_op, next, node, &list->hmap) {
+        map_op_destroy(map_op, type);
+    }
+    hmap_destroy(&list->hmap);
+    free(list);
+}
+
+static struct map_op*
+map_op_list_find(struct map_op_list *list, struct map_op *map_op,
+                 const struct ovsdb_type *type, size_t hash)
+{
+    struct map_op *found = NULL;
+    struct map_op *old;
+    HMAP_FOR_EACH_WITH_HASH(old, node, hash, &list->hmap) {
+        if (ovsdb_atom_equals(&old->datum->keys[0], &map_op->datum->keys[0],
+                              type->key.type)) {
+            found = old;
+            break;
+        }
+    }
+    return found;
+}
+
+/* Inserts 'map_op' into 'list'. Makes sure that any conflict with a previous
+ * map operation is resolved, so only one map operation is possible on each key
+ * per transactions. 'type' must be the type of the column over which the map
+ * operation will be applied. */
+void
+map_op_list_add(struct map_op_list *list, struct map_op *map_op,
+                const struct ovsdb_type *type)
+{
+    /* Check if there is a previous update with the same key. */
+    size_t hash;
+    struct map_op *prev_map_op;
+
+    hash = ovsdb_atom_hash(&map_op->datum->keys[0], type->key.type, 0);
+    prev_map_op = map_op_list_find(list, map_op, type, hash);
+    if (prev_map_op == NULL){
+        hmap_insert(&list->hmap, &map_op->node, hash);
+    } else {
+        if (prev_map_op->type == MAP_OP_INSERT &&
+            map_op->type == MAP_OP_DELETE) {
+            /* These operations cancel each other out. */
+            hmap_remove(&list->hmap, &prev_map_op->node);
+            map_op_destroy(prev_map_op, type);
+            map_op_destroy(map_op, type);
+        } else {
+            /* For any other case, the new update operation replaces
+             * the previous update operation. */
+            map_op_destroy_datum(prev_map_op, type);
+            prev_map_op->type = map_op->type;
+            prev_map_op->datum = map_op->datum;
+            free(map_op);
+        }
+    }
+}
+
+struct map_op*
+map_op_list_first(struct map_op_list *list)
+{
+    struct hmap_node *node = hmap_first(&list->hmap);
+    if (node == NULL) {
+        return NULL;
+    }
+    struct map_op *map_op = CONTAINER_OF(node, struct map_op, node);
+    return map_op;
+}
+
+struct map_op*
+map_op_list_next(struct map_op_list *list, struct map_op *map_op)
+{
+    struct hmap_node *node = hmap_next(&list->hmap, &map_op->node);
+    if (node == NULL) {
+        return NULL;
+    }
+    struct map_op *next = CONTAINER_OF(node, struct map_op, node);
+    return next;
+}
diff --git a/lib/ovsdb-map-op.h b/lib/ovsdb-map-op.h
new file mode 100644
index 0000000..8fd0b9e
--- /dev/null
+++ b/lib/ovsdb-map-op.h
@@ -0,0 +1,45 @@
+/* Copyright (C) 2016 Hewlett Packard Enterprise Development LP
+�* All Rights Reserved.
+�*
+�* Licensed under the Apache License, Version 2.0 (the "License"); you may
+�* not use this file except in compliance with the License. You may obtain
+�* a copy of the License at
+�*
+�*�����http://www.apache.org/licenses/LICENSE-2.0
+�*
+�* Unless required by applicable law or agreed to in writing, software
+�* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+�* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+�* License for the specific language governing permissions and limitations
+�* under the License.
+�*/
+
+#ifndef OVSDB_MAP_OP_H
+#define OVSDB_MAP_OP_H 1
+
+#include "ovsdb-data.h"
+
+enum map_op_type {
+    MAP_OP_UPDATE,
+    MAP_OP_INSERT,
+    MAP_OP_DELETE
+};
+
+struct map_op; /* Map Operation: a Partial Map Update */
+struct map_op_list; /* List of Map Operations */
+
+/* Map Operation functions */
+struct map_op* map_op_create(struct ovsdb_datum *, enum map_op_type);
+void map_op_destroy(struct map_op *, const struct ovsdb_type *);
+struct ovsdb_datum* map_op_datum(const struct map_op*);
+enum map_op_type map_op_type(const struct map_op*);
+
+/* Map Operation List functions */
+struct map_op_list* map_op_list_create(void);
+void map_op_list_destroy(struct map_op_list *, const struct ovsdb_type *);
+void map_op_list_add(struct map_op_list *, struct map_op *,
+                     const struct ovsdb_type *);
+struct map_op* map_op_list_first(struct map_op_list *);
+struct map_op* map_op_list_next(struct map_op_list *, struct map_op *);
+
+#endif /* ovsdb-map-op.h */
diff --git a/lib/ovsdb-pmu.c b/lib/ovsdb-pmu.c
deleted file mode 100644
index 42d6e14..0000000
--- a/lib/ovsdb-pmu.c
+++ /dev/null
@@ -1,117 +0,0 @@
-/* Copyright (C) 2016 Hewlett Packard Enterprise Development LP
-�* All Rights Reserved.
-�*
-�* Licensed under the Apache License, Version 2.0 (the "License"); you may
-�* not use this file except in compliance with the License. You may obtain
-�* a copy of the License at
-�*
-�*�����http://www.apache.org/licenses/LICENSE-2.0
-�*
-�* Unless required by applicable law or agreed to in writing, software
-�* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-�* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-�* License for the specific language governing permissions and limitations
-�* under the License.
-�*/
-
-#include <config.h>
-#include "ovsdb-pmu.h"
-#include "util.h"
-#include "hmap.h"
-#include "hash.h"
-
-/* PMU: Partial Map Update */
-struct pmu {
-    struct hmap_node node;
-    struct ovsdb_datum *datum;
-    enum pmu_operation operation;
-};
-
-/* PMUL: Partial Map Update List */
-struct pmul {
-    struct hmap hmap;
-};
-
-struct pmu*
-pmu_create(struct ovsdb_datum *datum, enum pmu_operation operation)
-{
-    struct pmu *pmu = xmalloc(sizeof *pmu);
-    pmu->node.hash = 0;
-    pmu->node.next = HMAP_NODE_NULL;
-    pmu->datum = datum;
-    pmu->operation = operation;
-    return pmu;
-}
-
-void
-pmu_destroy(struct pmu *pmu, const struct ovsdb_type *type)
-{
-    if (pmu->operation == PMU_DELETE){
-        struct ovsdb_type type_ = *type;
-        type_.value.type = OVSDB_TYPE_VOID;
-        ovsdb_datum_destroy(pmu->datum, &type_);
-    } else {
-        ovsdb_datum_destroy(pmu->datum, type);
-    }
-    free(pmu);
-}
-
-struct ovsdb_datum*
-pmu_datum(const struct pmu *pmu)
-{
-    return pmu->datum;
-}
-
-enum pmu_operation
-pmu_operation(const struct pmu *pmu)
-{
-    return pmu->operation;
-}
-
-struct pmul*
-pmul_create()
-{
-    struct pmul *list = xmalloc(sizeof *list);
-    hmap_init(&list->hmap);
-    return list;
-}
-
-void
-pmul_destroy(struct pmul *list, const struct ovsdb_type *type)
-{
-    struct pmu *pmu, *next;
-    HMAP_FOR_EACH_SAFE (pmu, next, node, &list->hmap) {
-        pmu_destroy(pmu, type);
-    }
-    hmap_destroy(&list->hmap);
-    free(list);
-}
-
-/* Inserts a new PMU into 'list'. */
-void
-pmul_add_pmu(struct pmul *list, struct pmu *pmu)
-{
-    size_t hash = hash_string(pmu->datum->keys[0].string, 0);
-    /* TODO: Check if there is another update with same key */
-    hmap_insert(&list->hmap, &pmu->node, hash);
-}
-
-struct pmu*
-pmul_first(struct pmul *list)
-{
-    struct hmap_node *node = hmap_first(&list->hmap);
-    if (node == NULL) {
-        return NULL;
-    }
-    struct pmu *pmu = CONTAINER_OF(node, struct pmu, node);
-    return pmu;
-}
-
-struct pmu* pmul_next(struct pmul *list, struct pmu *pmu){
-    struct hmap_node *node = hmap_next(&list->hmap, &pmu->node);
-    if (node == NULL) {
-        return NULL;
-    }
-    struct pmu *next = CONTAINER_OF(node, struct pmu, node);
-    return next;
-}
diff --git a/lib/ovsdb-pmu.h b/lib/ovsdb-pmu.h
deleted file mode 100644
index 17ca4a2..0000000
--- a/lib/ovsdb-pmu.h
+++ /dev/null
@@ -1,44 +0,0 @@
-/* Copyright (C) 2016 Hewlett Packard Enterprise Development LP
-�* All Rights Reserved.
-�*
-�* Licensed under the Apache License, Version 2.0 (the "License"); you may
-�* not use this file except in compliance with the License. You may obtain
-�* a copy of the License at
-�*
-�*�����http://www.apache.org/licenses/LICENSE-2.0
-�*
-�* Unless required by applicable law or agreed to in writing, software
-�* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-�* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-�* License for the specific language governing permissions and limitations
-�* under the License.
-�*/
-
-#ifndef OVSDB_PMU_H
-#define OVSDB_PMU_H 1
-
-#include "ovsdb-data.h"
-
-enum pmu_operation {
-    PMU_UPDATE,
-    PMU_INSERT,
-    PMU_DELETE
-};
-
-struct pmu; /* PMU: Partial Map Update */
-struct pmul; /* PMUL: Partial Map Update List */
-
-/* PMU: Partial Map Update functions */
-struct pmu* pmu_create(struct ovsdb_datum *, enum pmu_operation);
-void pmu_destroy(struct pmu *, const struct ovsdb_type *);
-struct ovsdb_datum* pmu_datum(const struct pmu*);
-enum pmu_operation pmu_operation(const struct pmu*);
-
-/* PMUL: Partial Map Update List functions */
-struct pmul* pmul_create(void);
-void pmul_destroy(struct pmul *, const struct ovsdb_type *);
-void pmul_add_pmu(struct pmul *list, struct pmu *pmu);
-struct pmu* pmul_first(struct pmul *);
-struct pmu* pmul_next(struct pmul *, struct pmu *);
-
-#endif /* ovsdb-pmu.h */
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 0d75113..0fcdb78 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -766,14 +766,14 @@ void
 %(s)s_update_%(c)s_setkey(const struct %(s)s *row, %(coltype)snew_key, %(valtype)snew_value)
 {
     struct ovsdb_datum *datum;
-    
+
     ovs_assert(inited);
-    
+
     datum = xmalloc(sizeof *datum);
     datum->n = 1;
     datum->keys = xmalloc(datum->n * sizeof *datum->keys);
     datum->values = xmalloc(datum->n * sizeof *datum->values);
-''' % {'s': structName, 'c': columnName,'coltype':column.type.key.toCType(prefix), 
+''' % {'s': structName, 'c': columnName,'coltype':column.type.key.toCType(prefix),
         'valtype':column.type.value.toCType(prefix), 'S': structName.upper(),
         'C': columnName.upper(), 't': tableName}
 
@@ -783,7 +783,7 @@ void
     ovsdb_idl_txn_write_partial_map(&row->header_,
                                     &%(s)s_columns[%(S)s_COL_%(C)s],
                                     datum);
-}''' % {'s': structName, 'c': columnName,'coltype':column.type.key.toCType(prefix), 
+}''' % {'s': structName, 'c': columnName,'coltype':column.type.key.toCType(prefix),
         'valtype':column.type.value.toCType(prefix), 'S': structName.upper(),
         'C': columnName.upper()}
                 print '''
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index 1d073aa..a133c57 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -105,6 +105,28 @@
           }
         }
       }
+    },
+    "simple2" : {
+        "columns" : {
+            "name" : {
+                "type": "string"
+            },
+            "smap" : {
+                "type": { "key" : "string",
+                          "value": "string",
+                          "min": 0,
+                          "max": "unlimited"
+                        }
+            },
+            "imap": {
+                "type" : { "key": { "type" : "integer",
+                                    "minInteger" : 0,
+                                    "maxInteger" : 4095},
+                            "value": { "type" : "string"},
+                          "min": 0, "max": "unlimited"
+                          }
+                }
+            }
+        }
     }
-  }
 }
diff --git a/tests/idltest2.ovsschema b/tests/idltest2.ovsschema
index 312c9cc..3cf164d 100644
--- a/tests/idltest2.ovsschema
+++ b/tests/idltest2.ovsschema
@@ -80,6 +80,28 @@
           }
         }
       }
-    }
+    },
+    "simple2" : {
+        "columns" : {
+            "name" : {
+                "type": "string"
+            },
+            "smap" : {
+                "type": { "key" : "string",
+                          "value": "string",
+                          "min": 0,
+                          "max": "unlimited"
+                        }
+            },
+            "imap": {
+                "type" : { "key": { "type" : "integer",
+                                    "minInteger" : 0,
+                                    "maxInteger" : 4095},
+                            "value": { "type" : "string"},
+                          "min": 0, "max": "unlimited"
+                          }
+                }
+            }
+        }
   }
 }
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index ebf82a5..d1d2a52 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -776,3 +776,37 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops],
 014: updated columns: ba i ia r ra s
 015: done
 ]])
+
+m4_define([OVSDB_CHECK_IDL_PARTIAL_UPDATE_MAP_COLUMN],
+  [AT_SETUP([$1 - C])
+   AT_KEYWORDS([ovsdb server idl partial update map column positive $5])
+   AT_CHECK([ovsdb-tool create db $abs_srcdir/idltest.ovsschema],
+                  [0], [stdout], [ignore])
+   AT_CHECK([ovsdb-server '-vPATTERN:console:ovsdb-server|%c|%m' --detach --no-chdir --pidfile="`pwd`"/pid --remote=punix:socket --unixctl="`pwd`"/unixctl db], [0], [ignore], [ignore])
+   m4_if([$2], [], [],
+     [AT_CHECK([ovsdb-client transact unix:socket $2], [0], [ignore], [ignore], [kill `cat pid`])])
+   AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 -c idl-partial-update-map-column unix:socket $3],
+            [0], [stdout], [ignore], [kill `cat pid`])
+   AT_CHECK([sort stdout | ${PERL} $srcdir/uuidfilt.pl]m4_if([$6],,, [[| $6]]),
+            [0], [$4], [], [kill `cat pid`])
+   OVSDB_SERVER_SHUTDOWN
+   AT_CLEANUP])
+
+OVSDB_CHECK_IDL_PARTIAL_UPDATE_MAP_COLUMN([map, simple2 idl-partial-update-map-column, initially populated],
+[['["idltest", {"op":"insert", "table":"simple2",
+                "row":{"name":"myString1","smap":["map",[["key1","value1"],["key2","value2"]]]} }]']
+],
+[],
+[[000: Getting records
+001: name=myString1 smap=[[key1 : value1],[key2 : value2]] imap=[]
+002: After insert element
+003: name=String2 smap=[[key1 : myList1],[key2 : value2]] imap=[[3 : myids2]]
+004: After insert duplicated element
+005: name=String2 smap=[[key1 : myList1],[key2 : value2]] imap=[[3 : myids2]]
+006: After delete element
+007: name=String2 smap=[[key2 : value2]] imap=[[3 : myids2]]
+008: After trying to delete a deleted element
+009: name=String2 smap=[[key2 : value2]] imap=[[3 : myids2]]
+010: End test
+]])
+
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 670a141..0ad03bd 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2178,6 +2178,109 @@ do_idl(struct ovs_cmdl_context *ctx)
     printf("%03d: done\n", step);
 }
 
+static void
+print_idl_row_simple2(const struct idltest_simple2 *s, int step)
+{
+    size_t i;
+    const struct ovsdb_datum *smap, *imap;
+
+    smap = idltest_simple2_get_smap(s, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
+    imap = idltest_simple2_get_imap(s, OVSDB_TYPE_INTEGER, OVSDB_TYPE_STRING);
+    printf("%03d: name=%s smap=[",
+           step, s->name);
+    for (i = 0; i < smap->n; i++) {
+        printf("[%s : %s]%s", smap->keys[i].string, smap->values[i].string,
+                i < smap->n-1? ",": "");
+    }
+    printf("] imap=[");
+    for (i = 0; i < imap->n; i++) {
+        printf("[%"PRId64" : %s]%s", imap->keys[i].integer, imap->values[i].string,
+                i < imap->n-1? ",":"");
+    }
+    printf("]\n");
+}
+
+static void
+dump_simple2(struct ovsdb_idl *idl, const struct idltest_simple2 *myRow, int step)
+{
+    IDLTEST_SIMPLE2_FOR_EACH(myRow, idl) {
+        print_idl_row_simple2(myRow, step);
+    }
+}
+
+
+static void
+do_idl_partial_update_map_column(struct ovs_cmdl_context *ctx)
+{
+    struct ovsdb_idl *idl;
+    struct ovsdb_idl_txn *myTxn;
+    const struct idltest_simple2 *myRow;
+    const struct ovsdb_datum *smap, *imap OVS_UNUSED;
+    int step = 0;
+    char key_to_delete[100];
+
+    idltest_init();
+    idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, false, true);
+    ovsdb_idl_add_table(idl, &idltest_table_simple2);
+    ovsdb_idl_add_column(idl, &idltest_simple2_col_name);
+    ovsdb_idl_add_column(idl, &idltest_simple2_col_smap);
+    ovsdb_idl_add_column(idl, &idltest_simple2_col_imap);
+    ovsdb_idl_get_initial_snapshot(idl);
+    setvbuf(stdout, NULL, _IONBF, 0);
+    ovsdb_idl_run(idl);
+
+    /* Display original data in table */
+    printf("%03d: Getting records\n", step++);
+    dump_simple2(idl, myRow, step++);
+
+    /* Insert new elements in different map columns */
+    myRow = idltest_simple2_first(idl);
+    myTxn = ovsdb_idl_txn_create(idl);
+    smap = idltest_simple2_get_smap(myRow, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
+    idltest_simple2_update_smap_setkey(myRow, "key1", "myList1");
+    imap = idltest_simple2_get_imap(myRow, OVSDB_TYPE_INTEGER, OVSDB_TYPE_STRING);
+    idltest_simple2_update_imap_setkey(myRow, 3, "myids2");
+    idltest_simple2_set_name(myRow, "String2");
+    ovsdb_idl_txn_commit_block(myTxn);
+    ovsdb_idl_txn_destroy(myTxn);
+    ovsdb_idl_get_initial_snapshot(idl);
+    printf("%03d: After insert element\n", step++);
+    dump_simple2(idl, myRow, step++);
+
+    /* Insert duplicate element */
+    myTxn = ovsdb_idl_txn_create(idl);
+    idltest_simple2_update_smap_setkey(myRow, "key1", "myList1");
+    ovsdb_idl_txn_commit_block(myTxn);
+    ovsdb_idl_txn_destroy(myTxn);
+    ovsdb_idl_get_initial_snapshot(idl);
+    printf("%03d: After insert duplicated element\n", step++);
+    dump_simple2(idl, myRow, step++);
+
+    /* deletes an element of a map column */
+    myRow = idltest_simple2_first(idl);
+    myTxn = ovsdb_idl_txn_create(idl);
+    smap = idltest_simple2_get_smap(myRow, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
+    strcpy(key_to_delete, smap->keys[0].string);
+    idltest_simple2_update_smap_delkey(myRow, smap->keys[0].string);
+    ovsdb_idl_txn_commit_block(myTxn);
+    ovsdb_idl_txn_destroy(myTxn);
+    ovsdb_idl_get_initial_snapshot(idl);
+    printf("%03d: After delete element\n", step++);
+    dump_simple2(idl, myRow, step++);
+
+    /* try to delete a deleted element of a map column */
+    myTxn = ovsdb_idl_txn_create(idl);
+    idltest_simple2_update_smap_delkey(myRow, key_to_delete);
+    ovsdb_idl_txn_commit_block(myTxn);
+    ovsdb_idl_txn_destroy(myTxn);
+    ovsdb_idl_get_initial_snapshot(idl);
+    printf("%03d: After trying to delete a deleted element\n", step++);
+    dump_simple2(idl, myRow, step++);
+
+    printf("%03d: End test\n", step);
+    return;
+}
+
 static struct ovs_cmdl_command all_commands[] = {
     { "log-io", NULL, 2, INT_MAX, do_log_io },
     { "default-atoms", NULL, 0, 0, do_default_atoms },
@@ -2206,6 +2309,7 @@ static struct ovs_cmdl_command all_commands[] = {
     { "execute", NULL, 2, INT_MAX, do_execute },
     { "trigger", NULL, 2, INT_MAX, do_trigger },
     { "idl", NULL, 1, INT_MAX, do_idl },
+    { "idl-partial-update-map-column", NULL, 1, INT_MAX, do_idl_partial_update_map_column },
     { "help", NULL, 0, INT_MAX, do_help },
     { NULL, NULL, 0, 0, NULL },
 };
-- 
1.9.1


More information about the dev mailing list