Thanks for the revision. I'm happy with this version.
I folded some mostly stylistic changes into patch 1. The one
non-stylistic change I made was to factor out error checking from
ovsdb_idl_txn_write_partial_map() and ovsdb_idl_txn_delete_partial_map()
into a common function is_valid_partial_update(). This reduced
duplication and made the code easier to read.
I'm appending the changes I folded in.
I applied these patches to master. Thank you!
diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h
index 1aafb00..04cf419 100644
--- a/lib/ovsdb-idl-provider.h
+++ b/lib/ovsdb-idl-provider.h
@@ -37,9 +37,8 @@ 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. */
+ unsigned long int *map_op_written; /* Bitmap of columns pending map ops. */
+ struct map_op_list **map_op_lists; /* Per-column map operations. */
/* Tracking data */
unsigned int change_seqno[OVSDB_IDL_CHANGE_MAX];
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index a0796bb..2b372cb 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1628,7 +1628,8 @@ ovsdb_idl_row_destroy(struct ovsdb_idl_row *row)
}
static void
-ovsdb_idl_destroy_all_map_op_lists(struct ovsdb_idl_row *row){
+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;
@@ -2235,7 +2236,7 @@ ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *row,
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 */
+ /* Find out if value really changed. */
struct ovsdb_datum *new_datum;
unsigned int pos;
new_datum = map_op_datum(map_op);
@@ -2249,13 +2250,13 @@ ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row
*row,
continue;
}
} else if (map_op_type(map_op) == MAP_OP_DELETE){
- /* Verify that there is a key to delete */
+ /* Verify that there is a key to delete. */
unsigned int pos;
pos = ovsdb_datum_find_key(old_datum,
&map_op_datum(map_op)->keys[0],
key_type);
if (pos == UINT_MAX) {
- /* No key to delete. Move on to next update. */
+ /* No key to delete. Move on to next update. */
VLOG_WARN("Trying to delete a key that doesn't "
"exist in the map.");
continue;
@@ -2277,7 +2278,7 @@ ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *row,
any_del = true;
}
- /* Generates an additional insert mutate for updates */
+ /* Generate 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],
@@ -2500,7 +2501,7 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
}
}
- /* Add Map Operation (Partial Map Updates). */
+ /* Add mutate operation, for partial map updates. */
if (row->map_op_written) {
struct json *op, *mutations;
bool any_mutations;
@@ -3343,7 +3344,7 @@ ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *row,
class = row->table->class;
column_idx = column - class->columns;
- /* Check if a Map Operation List exist for this column */
+ /* Check if a map operation list exists 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 *
@@ -3353,18 +3354,39 @@ ovsdb_idl_txn_add_map_op(struct ovsdb_idl_row *row,
row->map_op_lists[column_idx] = map_op_list_create();
}
- /* Add a Map Operation to the corresponding list */
+ /* 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 */
+ /* Add this row to transaction's list of rows. */
if (hmap_node_is_null(&row->txn_node)) {
hmap_insert(&row->table->idl->txn->txn_rows, &row->txn_node,
uuid_hash(&row->uuid));
}
}
+static bool
+is_valid_partial_update(const struct ovsdb_idl_row *row,
+ const struct ovsdb_idl_column *column,
+ struct ovsdb_datum *datum)
+{
+ /* Verify that this column is being monitored. */
+ unsigned int column_idx = column - row->table->class->columns;
+ if (!(row->table->modes[column_idx] & OVSDB_IDL_MONITOR)) {
+ VLOG_WARN("cannot partially update non-monitored column");
+ return false;
+ }
+
+ /* Verify that the update affects a single element. */
+ if (datum->n != 1) {
+ VLOG_WARN("invalid datum for partial update");
+ return false;
+ }
+
+ return true;
+}
+
/* 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
@@ -3380,36 +3402,20 @@ ovsdb_idl_txn_write_partial_map(const struct
ovsdb_idl_row *row_,
struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
enum ovsdb_atomic_type key_type;
enum map_op_type op_type;
- unsigned int column_idx, pos;
+ unsigned int pos;
const struct ovsdb_datum *old_datum;
- /* 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_write_partial_map(): Trying to update a non"
- "-monitored column.");
- ovsdb_datum_destroy(datum, &column->type);
- return;
- }
-
- if (datum->n != 1) {
- VLOG_WARN("ovsdb_idl_txn_write_partial_map(): Trying to set an invalid"
- " datum.");
+ if (!is_valid_partial_update(row, column, datum)) {
ovsdb_datum_destroy(datum, &column->type);
return;
}
- /* Find out if this is an insert or an update */
+ /* Find out if this is an insert or an update. */
key_type = column->type.key.type;
old_datum = ovsdb_idl_read(row, column);
pos = ovsdb_datum_find_key(old_datum, &datum->keys[0], key_type);
- if (pos == UINT_MAX) {
- /* Insert operation */
- op_type = MAP_OP_INSERT;
- } else {
- /* Update operation */
- op_type = MAP_OP_UPDATE;
- }
+ op_type = pos == UINT_MAX ? MAP_OP_INSERT : MAP_OP_UPDATE;
+
ovsdb_idl_txn_add_map_op(row, column, datum, op_type);
}
@@ -3426,28 +3432,13 @@ ovsdb_idl_txn_delete_partial_map(const struct
ovsdb_idl_row *row_,
struct ovsdb_datum *datum)
{
struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
- 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.");
- 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.");
+ if (!is_valid_partial_update(row, column, datum)) {
struct ovsdb_type type_ = column->type;
type_.value.type = OVSDB_TYPE_VOID;
ovsdb_datum_destroy(datum, &type_);
return;
}
-
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
index 67c3dee..58f43dc 100644
--- a/lib/ovsdb-map-op.c
+++ b/lib/ovsdb-map-op.c
@@ -33,7 +33,7 @@ struct map_op_list {
};
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 *,
+static struct map_op *map_op_list_find(struct map_op_list *, struct map_op *,
const struct ovsdb_type *, size_t);
struct map_op*
diff --git a/lib/ovsdb-map-op.h b/lib/ovsdb-map-op.h
index 8fd0b9e..140b0f3 100644
--- a/lib/ovsdb-map-op.h
+++ b/lib/ovsdb-map-op.h
@@ -29,17 +29,17 @@ 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);
+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*);
+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);
+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 *);
+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 */
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
