From 94fbe1aae29ecb71241cde7a8ed2688fa4621e1d Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 12 Apr 2012 08:25:10 -0700 Subject: [PATCH] ovsdb-idl: Improve ovsdb_idl_txn_increment() interface. The previous interface was just bizarre. Signed-off-by: Ben Pfaff --- lib/ovsdb-idl.c | 42 ++++++++++++++++++++++++++++-------------- lib/ovsdb-idl.h | 5 +++-- python/ovs/db/idl.py | 41 +++++++++++++++++++++++++++-------------- tests/ovsdb-idl.at | 4 ++-- tests/test-ovsdb.c | 15 ++++++++++++--- tests/test-ovsdb.py | 14 ++++++++++---- utilities/ovs-vsctl.c | 19 ++----------------- 7 files changed, 84 insertions(+), 56 deletions(-) diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 27a95760..dcbad105 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -94,9 +94,9 @@ struct ovsdb_idl_txn { unsigned int commit_seqno; /* Increments. */ - char *inc_table; - char *inc_column; - struct json *inc_where; + const char *inc_table; + const char *inc_column; + struct uuid inc_row; unsigned int inc_index; int64_t inc_new_value; @@ -1185,7 +1185,6 @@ ovsdb_idl_txn_create(struct ovsdb_idl *idl) txn->inc_table = NULL; txn->inc_column = NULL; - txn->inc_where = NULL; hmap_init(&txn->inserted_rows); @@ -1216,14 +1215,30 @@ ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *txn) txn->dry_run = true; } +/* Causes 'txn', when committed, to increment the value of 'column' within + * 'row' by 1. 'column' must have an integer type. After 'txn' commits + * successfully, the client may retrieve the final (incremented) value of + * 'column' with ovsdb_idl_txn_get_increment_new_value(). + * + * The client could accomplish something similar with ovsdb_idl_read(), + * ovsdb_idl_txn_verify() and ovsdb_idl_txn_write(), or with ovsdb-idlc + * generated wrappers for these functions. However, ovsdb_idl_txn_increment() + * will never (by itself) fail because of a verify error. + * + * The intended use is for incrementing the "next_cfg" column in the + * Open_vSwitch table. */ void -ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn, const char *table, - const char *column, const struct json *where) +ovsdb_idl_txn_increment(struct ovsdb_idl_txn *txn, + const struct ovsdb_idl_row *row, + const struct ovsdb_idl_column *column) { assert(!txn->inc_table); - txn->inc_table = xstrdup(table); - txn->inc_column = xstrdup(column); - txn->inc_where = where ? json_clone(where) : json_array_create_empty(); + assert(column->type.key.type == OVSDB_TYPE_INTEGER); + assert(column->type.value.type == OVSDB_TYPE_VOID); + + txn->inc_table = row->table->class->name; + txn->inc_column = column->name; + txn->inc_row = row->uuid; } void @@ -1238,9 +1253,6 @@ ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *txn) ovsdb_idl_txn_abort(txn); ds_destroy(&txn->comment); free(txn->error); - free(txn->inc_table); - free(txn->inc_column); - json_destroy(txn->inc_where); HMAP_FOR_EACH_SAFE (insert, next, hmap_node, &txn->inserted_rows) { free(insert); } @@ -1528,7 +1540,8 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) json_object_put_string(op, "op", "mutate"); json_object_put_string(op, "table", txn->inc_table); json_object_put(op, "where", - substitute_uuids(json_clone(txn->inc_where), txn)); + substitute_uuids(where_uuid_equals(&txn->inc_row), + txn)); json_object_put(op, "mutations", json_array_create_1( json_array_create_3( @@ -1541,7 +1554,8 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) json_object_put_string(op, "op", "select"); json_object_put_string(op, "table", txn->inc_table); json_object_put(op, "where", - substitute_uuids(json_clone(txn->inc_where), txn)); + substitute_uuids(where_uuid_equals(&txn->inc_row), + txn)); json_object_put(op, "columns", json_array_create_1(json_string_create( txn->inc_column))); diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index 9b49e623..34ccf061 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -134,8 +134,9 @@ struct ovsdb_idl_txn *ovsdb_idl_txn_create(struct ovsdb_idl *); void ovsdb_idl_txn_add_comment(struct ovsdb_idl_txn *, const char *, ...) PRINTF_FORMAT (2, 3); void ovsdb_idl_txn_set_dry_run(struct ovsdb_idl_txn *); -void ovsdb_idl_txn_increment(struct ovsdb_idl_txn *, const char *table, - const char *column, const struct json *where); +void ovsdb_idl_txn_increment(struct ovsdb_idl_txn *, + const struct ovsdb_idl_row *, + const struct ovsdb_idl_column *); void ovsdb_idl_txn_destroy(struct ovsdb_idl_txn *); void ovsdb_idl_txn_wait(const struct ovsdb_idl_txn *); enum ovsdb_idl_txn_status ovsdb_idl_txn_commit(struct ovsdb_idl_txn *); diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py index 9760fc64..fba20dcb 100644 --- a/python/ovs/db/idl.py +++ b/python/ovs/db/idl.py @@ -604,6 +604,21 @@ class Row(object): self.__dict__["_changes"] = None del self._table.rows[self.uuid] + def increment(self, column_name): + """Causes the transaction, when committed, to increment the value of + 'column_name' within this row by 1. 'column_name' must have an integer + type. After the transaction commits successfully, the client may + retrieve the final (incremented) value of 'column_name' with + Transaction.get_increment_new_value(). + + The client could accomplish something similar by reading and writing + and verify()ing columns. However, increment() will never (by itself) + cause a transaction to fail because of a verify error. + + The intended use is for incrementing the "next_cfg" column in + the Open_vSwitch table.""" + self._idl.txn._increment(self, column_name) + def _uuid_name_from_uuid(uuid): return "row%s" % str(uuid).replace("-", "_") @@ -666,9 +681,8 @@ class Transaction(object): self._comments = [] self._commit_seqno = self.idl.change_seqno - self._inc_table = None + self._inc_row = None self._inc_column = None - self._inc_where = None self._inserted_rows = {} # Map from UUID to _InsertedRow @@ -679,12 +693,6 @@ class Transaction(object): relatively human-readable form.)""" self._comments.append(comment) - def increment(self, table, column, where): - assert not self._inc_table - self._inc_table = table - self._inc_column = column - self._inc_where = where - def wait(self, poller): if self._status not in (Transaction.UNCOMMITTED, Transaction.INCOMPLETE): @@ -803,18 +811,18 @@ class Transaction(object): operations.append(op) # Add increment. - if self._inc_table and any_updates: + if self._inc_row and any_updates: self._inc_index = len(operations) - 1 operations.append({"op": "mutate", - "table": self._inc_table, + "table": self._inc_row._table.name, "where": self._substitute_uuids( - self._inc_where), + _where_uuid_equals(self._inc_row.uuid)), "mutations": [[self._inc_column, "+=", 1]]}) operations.append({"op": "select", - "table": self._inc_table, + "table": self._inc_row._table.name, "where": self._substitute_uuids( - self._inc_where), + _where_uuid_equals(self._inc_row.uuid)), "columns": [self._inc_column]}) # Add comment. @@ -898,6 +906,11 @@ class Transaction(object): return inserted_row.real return None + def _increment(self, row, column): + assert not self._inc_row + self._inc_row = row + self._inc_column = column + def _write(self, row, column, datum): assert row._changes is not None @@ -978,7 +991,7 @@ class Transaction(object): vlog.warn("operation reply is not JSON null or object") if not soft_errors and not hard_errors and not lock_errors: - if self._inc_table and not self.__process_inc_reply(ops): + if self._inc_row and not self.__process_inc_reply(ops): hard_errors = True for insert in self._inserted_rows.itervalues(): diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index 2cfc91c6..91f16719 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -221,7 +221,7 @@ OVSDB_CHECK_IDL([simple idl, handling verification failure], 000: i=1 r=2 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2> 001: commit, status=success 002: {"error":null,"result":[{"count":1}]} -003: commit, status=try again now +003: commit, status=try again 004: i=0 r=0 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> 004: i=1 r=5 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2> 005: commit, status=success @@ -235,7 +235,7 @@ OVSDB_CHECK_IDL([simple idl, increment operation], {"op": "insert", "table": "simple", "row": {}}]']], - [['set 0 r 2.0, increment simple i']], + [['set 0 r 2.0, increment 0']], [[000: i=0 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> 001: commit, status=success, increment=1 002: i=1 r=2 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index cc01bbe3..4e2e416c 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -1825,10 +1825,19 @@ idl_set(struct ovsdb_idl *idl, char *commands, int step) arg2); } } else if (!strcmp(name, "increment")) { - if (!arg2 || arg3) { - ovs_fatal(0, "\"increment\" command requires 2 arguments"); + const struct idltest_simple *s; + + if (!arg1 || arg2) { + ovs_fatal(0, "\"increment\" command requires 1 argument"); } - ovsdb_idl_txn_increment(txn, arg1, arg2, NULL); + + s = idltest_find_simple(idl, atoi(arg1)); + if (!s) { + ovs_fatal(0, "\"set\" command asks for nonexistent " + "i=%d", atoi(arg1)); + } + + ovsdb_idl_txn_increment(txn, &s->header_, &idltest_simple_col_i); increment = true; } else if (!strcmp(name, "abort")) { ovsdb_idl_txn_abort(txn); diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py index b0e42a35..77b3a2c1 100644 --- a/tests/test-ovsdb.py +++ b/tests/test-ovsdb.py @@ -1,4 +1,4 @@ -# Copyright (c) 2009, 2010, 2011 Nicira Networks +# Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -292,11 +292,17 @@ def idl_set(idl, commands, step): '"%s"\n' % args[1]) sys.exit(1) elif name == "increment": - if len(args) != 2: - sys.stderr.write('"increment" command requires 2 arguments\n') + if len(args) != 1: + sys.stderr.write('"increment" command requires 1 argument\n') + sys.exit(1) + + s = idltest_find_simple(idl, int(args[0])) + if not s: + sys.stderr.write('"set" command asks for nonexistent i=%d\n' + % int(args[0])) sys.exit(1) - txn.increment(args[0], args[1], []) + s.increment("i") increment = True elif name == "abort": txn.abort() diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index d22a518d..a5bd6474 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -3605,20 +3605,6 @@ cmd_wait_until(struct vsctl_context *ctx) } } -static struct json * -where_uuid_equals(const struct uuid *uuid) -{ - return - json_array_create_1( - json_array_create_3( - json_string_create("_uuid"), - json_string_create("=="), - json_array_create_2( - json_string_create("uuid"), - json_string_create_nocopy( - xasprintf(UUID_FMT, UUID_ARGS(uuid)))))); -} - static void vsctl_context_init(struct vsctl_context *ctx, struct vsctl_command *command, struct ovsdb_idl *idl, struct ovsdb_idl_txn *txn, @@ -3701,9 +3687,8 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands, } if (wait_for_reload) { - struct json *where = where_uuid_equals(&ovs->header_.uuid); - ovsdb_idl_txn_increment(txn, "Open_vSwitch", "next_cfg", where); - json_destroy(where); + ovsdb_idl_txn_increment(txn, &ovs->header_, + &ovsrec_open_vswitch_col_next_cfg); } symtab = ovsdb_symbol_table_create(); -- 2.30.2