| Message ID | 20220629165720.3885265-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] ovsdb idl: Add the support to specify the uuid for row insert. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Numan Siddique, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 118 characters long (recommended limit is 79) #104 FILE: lib/db-ctl-base.man:206: .IP "[\fB\-\-id=@\fIname\fR or \fB\-\-id=\fIuuid\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." Lines checked: 469, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 6/29/22 18:57, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > ovsdb-server allows the OVSDB clients to specify the uuid for > the row inserts [1]. The C IDL client library is missing this > feature. This patch adds this support. > > For each schema table, a new function is generated - > <schema_table>insert_persistent_uuid(txn, uuid) and the users > of IDL client library can make use of this function. > > ovs-vsctl and other derivatives of ctl now supports the same > in the generic 'create' command with the option "--id=<UUID>". > > [1] - a529e3cd1f("ovsdb-server: Allow OVSDB clients to specify the UUID for inserted rows.:) > > Signed-off-by: Numan Siddique <numans@ovn.org> > Acked-by: Adrian Moreno <amorenoz@redhat.com> > Acked-by: Han Zhou <hzhou@ovn.org> > --- Hi, Numan. Thanks for the patch. It looks OK in general, see some comments inline. > > v3 -> v4 > --- > * Added an entry in python/TODO.rst. > > v2 -> v3 > ---- > * Addressed review comments from Han > - Added test case for --id ctl option > > v1 -> v2 > ----- > * Addressed review comments from Adrian Moreno > * Added the support in generic 'create' command to specify the uuid in > --id option. > > > lib/db-ctl-base.c | 38 ++++++++++++------ > lib/db-ctl-base.man | 5 ++- > lib/db-ctl-base.xml | 4 ++ > lib/ovsdb-idl-provider.h | 1 + > lib/ovsdb-idl.c | 86 +++++++++++++++++++++++++++++----------- > lib/ovsdb-idl.h | 3 ++ > ovsdb/ovsdb-idlc.in | 15 +++++++ > python/TODO.rst | 2 + > tests/ovs-vsctl.at | 25 ++++++++++++ > tests/ovsdb-idl.at | 27 +++++++++++++ > tests/test-ovsdb.c | 59 +++++++++++++++++++++++++++ > 11 files changed, 229 insertions(+), 36 deletions(-) We need a NEWS entry for this. > > diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c > index 707456158..f39e090a0 100644 > --- a/lib/db-ctl-base.c > +++ b/lib/db-ctl-base.c > @@ -1732,28 +1732,42 @@ cmd_create(struct ctl_context *ctx) > const struct ovsdb_idl_row *row; > const struct uuid *uuid = NULL; > int i; > + struct uuid uuid_; > + bool persist_uuid = false; Reverse x-mass tree, please. > > ctx->error = get_table(table_name, &table); > if (ctx->error) { > return; > } > + > if (id) { > - struct ovsdb_symbol *symbol = NULL; > + if (uuid_from_string(&uuid_, id)) { > + uuid = &uuid_; > + persist_uuid = true; > + } else { > + struct ovsdb_symbol *symbol = NULL; > > - ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL); > - if (ctx->error) { > - return; > - } > - if (table->is_root) { > - /* This table is in the root set, meaning that rows created in it > - * won't disappear even if they are unreferenced, so disable > - * warnings about that by pretending that there is a reference. */ > - symbol->strong_ref = true; > + ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL); > + if (ctx->error) { > + return; > + } > + if (table->is_root) { > + /* This table is in the root set, meaning that rows created in > + * it won't disappear even if they are unreferenced, so disable > + * warnings about that by pretending that there is a > + * reference. */ Subsequent lines of a comment block are 1 spaece off. > + symbol->strong_ref = true; > + } > + uuid = &symbol->uuid; > } > - uuid = &symbol->uuid; > } > > - row = ovsdb_idl_txn_insert(ctx->txn, table, uuid); > + if (persist_uuid) { > + row = ovsdb_idl_txn_insert_persist_uuid(ctx->txn, table, uuid); > + } else { > + row = ovsdb_idl_txn_insert(ctx->txn, table, uuid); > + } > + > for (i = 2; i < ctx->argc; i++) { > ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab); > if (ctx->error) { > diff --git a/lib/db-ctl-base.man b/lib/db-ctl-base.man > index 9c786f298..4dc165f94 100644 > --- a/lib/db-ctl-base.man > +++ b/lib/db-ctl-base.man > @@ -203,7 +203,7 @@ Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not > exist. With \fB\-\-if-exists\fR, this command does nothing if > \fIrecord\fR does not exist. > . > -.IP "[\fB\-\-id=@\fIname\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." > +.IP "[\fB\-\-id=@\fIname\fR or \fB\-\-id=\fIuuid\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." I think, it should be [--id=(@name|uuid)] > Creates a new record in \fItable\fR and sets the initial values of > each \fIcolumn\fR. Columns not explicitly set will receive their > default values. Outputs the UUID of the new row. > @@ -212,6 +212,9 @@ If \fB@\fIname\fR is specified, then the UUID for the new row may be > referred to by that name elsewhere in the same \fB\*(PN\fR > invocation in contexts where a UUID is expected. Such references may > precede or follow the \fBcreate\fR command. > +.IP > +If a valid \fIuuid\fR is specified, then it is used as the UUID > +of the new row. > . > .RS > .IP "Caution (ovs-vsctl as example)" > diff --git a/lib/db-ctl-base.xml b/lib/db-ctl-base.xml > index f6efe98ea..4f63aa7f7 100644 > --- a/lib/db-ctl-base.xml > +++ b/lib/db-ctl-base.xml > @@ -323,6 +323,10 @@ The argument line needs an update in this file as well. > invocation in contexts where a UUID is expected. Such references may > precede or follow the <code>create</code> command. > </p> > + <p> > + If a valid <var>uuid</var> is specified, then it is used as the > + UUID of the new row. > + </p> > <dl> > <dt>Caution (ovs-vsctl as example)</dt> > <dd> > diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > index 8797686f9..332c4075b 100644 > --- a/lib/ovsdb-idl-provider.h > +++ b/lib/ovsdb-idl-provider.h > @@ -74,6 +74,7 @@ struct ovsdb_idl_row { > struct ovs_list dst_arcs; /* Backward arcs (ovsdb_idl_arc.dst_node). */ > struct ovsdb_idl_table *table; /* Containing table. */ > struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */ > + bool persist_uuid; /* persist 'uuid' during insert txn if set. */ Comments should start with a capital letter. > bool parsed; /* Whether the row is parsed. */ > struct ovs_list reparse_node; /* Rows that needs to be re-parsed due to > * insertion of a referenced row. */ > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 3b8741408..9ddb49273 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -2849,11 +2849,14 @@ substitute_uuids(struct json *json, const struct ovsdb_idl_txn *txn) > > row = ovsdb_idl_txn_get_row(txn, &uuid); > if (row && !row->old_datum && row->new_datum) { > - json_destroy(json); > - > - return json_array_create_2( > - json_string_create("named-uuid"), > - json_string_create_nocopy(ovsdb_data_row_name(&uuid))); > + if (row->persist_uuid) { > + return json; > + } else { > + json_destroy(json); > + return json_array_create_2( > + json_string_create("named-uuid"), > + json_string_create_nocopy(ovsdb_data_row_name(&uuid))); > + } > } > } > > @@ -3278,9 +3281,19 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) > > any_updates = true; > > - json_object_put(op, "uuid-name", > - json_string_create_nocopy( > - ovsdb_data_row_name(&row->uuid))); > + char *uuid_json; > + struct json *value; > + if (row->persist_uuid) { > + uuid_json = "uuid"; > + value = json_string_create_nocopy( > + xasprintf(UUID_FMT, UUID_ARGS(&row->uuid))); > + } else { > + uuid_json = "uuid-name"; > + value = json_string_create_nocopy( > + ovsdb_data_row_name(&row->uuid)); > + } > + > + json_object_put(op, uuid_json, value); > > insert = xmalloc(sizeof *insert); > insert->dummy = row->uuid; > @@ -3765,6 +3778,32 @@ ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_) > row->new_datum = NULL; > } > > +static const struct ovsdb_idl_row * > +ovsdb_idl_txn_insert__(struct ovsdb_idl_txn *txn, > + const struct ovsdb_idl_table_class *class, > + const struct uuid *uuid, > + bool persist_uuid) > +{ > + struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class); > + Should we also ovs_assert(uuid || !persist_uuid) ? > + if (uuid) { > + ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); > + row->uuid = *uuid; > + row->persist_uuid = persist_uuid; > + } else { > + uuid_generate(&row->uuid); > + row->persist_uuid = false; > + } > + > + row->table = ovsdb_idl_table_from_class(txn->idl, class); > + row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum); > + hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); > + hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); > + ovsdb_idl_add_to_indexes(row); > + > + return row; > +} > + > /* Inserts and returns a new row in the table with the specified 'class' in the > * database with open transaction 'txn'. > * > @@ -3782,22 +3821,23 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn, > const struct ovsdb_idl_table_class *class, > const struct uuid *uuid) > { > - struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class); > - > - if (uuid) { > - ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); > - row->uuid = *uuid; > - } else { > - uuid_generate(&row->uuid); > - } > - > - row->table = ovsdb_idl_table_from_class(txn->idl, class); > - row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum); > - hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); > - hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); > - ovsdb_idl_add_to_indexes(row); > + return ovsdb_idl_txn_insert__(txn, class, uuid, false); > +} > > - return row; > +/* Inserts and returns a new row in the table with the specified 'class' in the > + * database with open transaction 'txn'. > + * > + * The new row is assigned the specified UUID (which cannot be null). > + * > + * Usually this function is used indirectly through one of the > + * "insert_persist_uuid" functions generated by ovsdb-idlc. */ > +const struct ovsdb_idl_row * > +ovsdb_idl_txn_insert_persist_uuid(struct ovsdb_idl_txn *txn, > + const struct ovsdb_idl_table_class *class, > + const struct uuid *uuid) > +{ > + ovs_assert(uuid); > + return ovsdb_idl_txn_insert__(txn, class, uuid, true); > } > > static void > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > index fbd9f671a..9a3e19f20 100644 > --- a/lib/ovsdb-idl.h > +++ b/lib/ovsdb-idl.h > @@ -375,6 +375,9 @@ void ovsdb_idl_txn_delete(const struct ovsdb_idl_row *); > const struct ovsdb_idl_row *ovsdb_idl_txn_insert( > struct ovsdb_idl_txn *, const struct ovsdb_idl_table_class *, > const struct uuid *); > +const struct ovsdb_idl_row *ovsdb_idl_txn_insert_persist_uuid( > + struct ovsdb_idl_txn *txn, const struct ovsdb_idl_table_class *class, > + const struct uuid *uuid); > > struct ovsdb_idl *ovsdb_idl_txn_get_idl (struct ovsdb_idl_txn *); > void ovsdb_idl_get_initial_snapshot(struct ovsdb_idl *); > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in > index 13c535939..7e09497ea 100755 > --- a/ovsdb/ovsdb-idlc.in > +++ b/ovsdb/ovsdb-idlc.in > @@ -362,6 +362,8 @@ struct %(s)s *%(s)s_cursor_data(struct ovsdb_idl_cursor *); > void %(s)s_init(struct %(s)s *); > void %(s)s_delete(const struct %(s)s *); > struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *); > +struct %(s)s *%(s)s_insert_persist_uuid( > + struct ovsdb_idl_txn *txn, const struct uuid *uuid); > > /* Returns true if the tracked column referenced by 'enum %(s)s_column_id' of > * the row referenced by 'struct %(s)s *' was updated since the last change > @@ -809,6 +811,19 @@ struct %(s)s * > return %(s)s_cast(ovsdb_idl_txn_insert(txn, &%(p)stable_%(tl)s, NULL)); > } > > +/* Inserts and returns a new row in the table "%(t)s" in the database > + * with open transaction 'txn'. > + * > + * The new row is assigned the UUID specified in the 'uuid' parameter > + * (which cannot be null). ovsdb-server will try to assign the same > + * UUID when 'txn' is committed. */ > +struct %(s)s * > +%(s)s_insert_persist_uuid(struct ovsdb_idl_txn *txn, const struct uuid *uuid) > +{ > + return %(s)s_cast(ovsdb_idl_txn_insert_persist_uuid( > + txn, &%(p)stable_%(tl)s, uuid)); > +} > + > bool > %(s)s_is_updated(const struct %(s)s *row, enum %(s)s_column_id column) > { > diff --git a/python/TODO.rst b/python/TODO.rst > index 3a53489f1..a0b3e807a 100644 > --- a/python/TODO.rst > +++ b/python/TODO.rst > @@ -32,3 +32,5 @@ Python Bindings To-do List > > * Support write-only-changed monitor mode (equivalent of > OVSDB_IDL_WRITE_CHANGED_ONLY). > + > + * Support accepting the uuid for row inserts. > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index d6cd2c084..abf4fb9cf 100644 > --- a/tests/ovs-vsctl.at > +++ b/tests/ovs-vsctl.at > @@ -1710,3 +1710,28 @@ ingress_policing_kpkts_rate: 100 > ]) > OVS_VSCTL_CLEANUP > AT_CLEANUP > + > +AT_SETUP([ovs-vsctl create bridge with uuid]) > +AT_KEYWORDS([create bridge with uuid]) > +OVS_VSCTL_SETUP > + > +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 create bridge \ > +name=tst0 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [0],[dnl > +c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > +]) > + > +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 create bridge \ > +name=tst1 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [1], [ignore], [ignore]) > + > +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge], [0], [dnl > +c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > +tst0 > +]) > + > +ovs-vsctl --no-wait --id=@a create bridge \ > +name=tst1 -- add open . bridges @a > + > +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0], [ignore]) > + > +OVS_VSCTL_CLEANUP > +AT_CLEANUP > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > index d1cfa59c5..28c032105 100644 > --- a/tests/ovsdb-idl.at > +++ b/tests/ovsdb-idl.at > @@ -2466,3 +2466,30 @@ unix:socket2 remote has col id in table simple7 > > OVSDB_SERVER_SHUTDOWN > AT_CLEANUP > + > +AT_SETUP([idl creating rows with persistent uuid - C]) > +AT_KEYWORDS([ovsdb client idl txn]) > + > +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest.ovsschema"]) > +AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl-txn-persistent-uuid unix:socket], > + [0], [stdout], [stderr]) > +AT_CHECK([sort stdout], [0], > + [[000: After inserting simple, simple3 and simple4 > +001: table simple3: name=simple3 uset=[] uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222 > +001: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > +001: table simple: i=0 r=0 b=false s=simple u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333 > +002: After inserting simple with same uuid > +003: table simple3: name=simple3 uset=[] uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222 > +003: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > +003: table simple: i=0 r=0 b=false s=simple u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333 > +004: End test > +]]) > + > +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl > +test-ovsdb|ovsdb_idl|transaction error: {"details":"This UUID would dnl > +duplicate a UUID already present within the table or deleted within dnl > +the same transaction.","error":"duplicate uuid","syntax":"\"c5cc12f8-eaa1-43a7-8a73-bccd18df3333\""} > +]) > + > +OVSDB_SERVER_SHUTDOWN > +AT_CLEANUP > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > index 0fef1f978..515a3015d 100644 > --- a/tests/test-ovsdb.c > +++ b/tests/test-ovsdb.c > @@ -3389,6 +3389,63 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx) > ovsdb_idl_destroy(idl); > } > > +static void > +do_idl_txn_persistent_uuid(struct ovs_cmdl_context *ctx) > +{ > + struct ovsdb_idl *idl; > + struct ovsdb_idl_txn *myTxn; > + int step = 0; > + > + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); > + ovsdb_idl_get_initial_snapshot(idl); > + ovsdb_idl_run(idl); > + > + myTxn = ovsdb_idl_txn_create(idl); > + > + struct uuid uuid1; > + uuid_from_string(&uuid1, "c5cc12f8-eaa1-43a7-8a73-bccd18df1111"); > + > + struct uuid uuid2; > + uuid_from_string(&uuid2, "c5cc12f8-eaa1-43a7-8a73-bccd18df2222"); > + > + struct idltest_simple4 *simple4_row = > + idltest_simple4_insert_persist_uuid(myTxn, &uuid1); > + idltest_simple4_set_name(simple4_row, "simple4"); > + > + struct idltest_simple3 *simple3_row = > + idltest_simple3_insert_persist_uuid(myTxn, &uuid2); > + idltest_simple3_set_name(simple3_row, "simple3"); > + idltest_simple3_set_uref(simple3_row, &simple4_row, 1); > + > + struct uuid uuid3; > + uuid_from_string(&uuid3, "c5cc12f8-eaa1-43a7-8a73-bccd18df3333"); > + > + struct idltest_simple *simple_row = > + idltest_simple_insert_persist_uuid(myTxn, &uuid3); > + idltest_simple_set_s(simple_row, "simple"); > + > + ovsdb_idl_txn_commit_block(myTxn); > + ovsdb_idl_txn_destroy(myTxn); > + ovsdb_idl_get_initial_snapshot(idl); > + printf("%03d: After inserting simple, simple3 and simple4\n", step++); > + print_idl(idl, step++, false); > + > + /* Create another txn, insert the row in simple table with the existing > + * uuid. */ > + myTxn = ovsdb_idl_txn_create(idl); > + simple_row = > + idltest_simple_insert_persist_uuid(myTxn, &uuid3); > + idltest_simple_set_s(simple_row, "simple_foo"); > + ovsdb_idl_txn_commit_block(myTxn); > + ovsdb_idl_txn_destroy(myTxn); > + ovsdb_idl_get_initial_snapshot(idl); > + printf("%03d: After inserting simple with same uuid\n", step++); > + print_idl(idl, step++, false); > + > + ovsdb_idl_destroy(idl); > + printf("%03d: End test\n", step++); > +} > + > static struct ovs_cmdl_command all_commands[] = { > { "log-io", NULL, 2, INT_MAX, do_log_io, OVS_RO }, > { "default-atoms", NULL, 0, 0, do_default_atoms, OVS_RO }, > @@ -3429,6 +3486,8 @@ static struct ovs_cmdl_command all_commands[] = { > do_idl_partial_update_set_column, OVS_RO }, > { "idl-table-column-check", NULL, 2, 2, > do_idl_table_column_check, OVS_RO }, > + { "idl-txn-persistent-uuid", NULL, 1, INT_MAX, > + do_idl_txn_persistent_uuid, OVS_RO }, > { "help", NULL, 0, INT_MAX, do_help, OVS_RO }, > { NULL, NULL, 0, 0, NULL, OVS_RO }, > };
On Tue, Jul 12, 2022 at 6:02 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 6/29/22 18:57, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > ovsdb-server allows the OVSDB clients to specify the uuid for > > the row inserts [1]. The C IDL client library is missing this > > feature. This patch adds this support. > > > > For each schema table, a new function is generated - > > <schema_table>insert_persistent_uuid(txn, uuid) and the users > > of IDL client library can make use of this function. > > > > ovs-vsctl and other derivatives of ctl now supports the same > > in the generic 'create' command with the option "--id=<UUID>". > > > > [1] - a529e3cd1f("ovsdb-server: Allow OVSDB clients to specify the UUID for inserted rows.:) > > > > Signed-off-by: Numan Siddique <numans@ovn.org> > > Acked-by: Adrian Moreno <amorenoz@redhat.com> > > Acked-by: Han Zhou <hzhou@ovn.org> > > --- > > Hi, Numan. Thanks for the patch. It looks OK in general, > see some comments inline. Thanks for the review comments. > > > > > v3 -> v4 > > --- > > * Added an entry in python/TODO.rst. > > > > v2 -> v3 > > ---- > > * Addressed review comments from Han > > - Added test case for --id ctl option > > > > v1 -> v2 > > ----- > > * Addressed review comments from Adrian Moreno > > * Added the support in generic 'create' command to specify the uuid in > > --id option. > > > > > > lib/db-ctl-base.c | 38 ++++++++++++------ > > lib/db-ctl-base.man | 5 ++- > > lib/db-ctl-base.xml | 4 ++ > > lib/ovsdb-idl-provider.h | 1 + > > lib/ovsdb-idl.c | 86 +++++++++++++++++++++++++++++----------- > > lib/ovsdb-idl.h | 3 ++ > > ovsdb/ovsdb-idlc.in | 15 +++++++ > > python/TODO.rst | 2 + > > tests/ovs-vsctl.at | 25 ++++++++++++ > > tests/ovsdb-idl.at | 27 +++++++++++++ > > tests/test-ovsdb.c | 59 +++++++++++++++++++++++++++ > > 11 files changed, 229 insertions(+), 36 deletions(-) > > We need a NEWS entry for this. > > > > > diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c > > index 707456158..f39e090a0 100644 > > --- a/lib/db-ctl-base.c > > +++ b/lib/db-ctl-base.c > > @@ -1732,28 +1732,42 @@ cmd_create(struct ctl_context *ctx) > > const struct ovsdb_idl_row *row; > > const struct uuid *uuid = NULL; > > int i; > > + struct uuid uuid_; > > + bool persist_uuid = false; > > Reverse x-mass tree, please. Done in v5 > > > > > ctx->error = get_table(table_name, &table); > > if (ctx->error) { > > return; > > } > > + > > if (id) { > > - struct ovsdb_symbol *symbol = NULL; > > + if (uuid_from_string(&uuid_, id)) { > > + uuid = &uuid_; > > + persist_uuid = true; > > + } else { > > + struct ovsdb_symbol *symbol = NULL; > > > > - ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL); > > - if (ctx->error) { > > - return; > > - } > > - if (table->is_root) { > > - /* This table is in the root set, meaning that rows created in it > > - * won't disappear even if they are unreferenced, so disable > > - * warnings about that by pretending that there is a reference. */ > > - symbol->strong_ref = true; > > + ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL); > > + if (ctx->error) { > > + return; > > + } > > + if (table->is_root) { > > + /* This table is in the root set, meaning that rows created in > > + * it won't disappear even if they are unreferenced, so disable > > + * warnings about that by pretending that there is a > > + * reference. */ > > Subsequent lines of a comment block are 1 spaece off. Done in v5. > > > + symbol->strong_ref = true; > > + } > > + uuid = &symbol->uuid; > > } > > - uuid = &symbol->uuid; > > } > > > > - row = ovsdb_idl_txn_insert(ctx->txn, table, uuid); > > + if (persist_uuid) { > > + row = ovsdb_idl_txn_insert_persist_uuid(ctx->txn, table, uuid); > > + } else { > > + row = ovsdb_idl_txn_insert(ctx->txn, table, uuid); > > + } > > + > > for (i = 2; i < ctx->argc; i++) { > > ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab); > > if (ctx->error) { > > diff --git a/lib/db-ctl-base.man b/lib/db-ctl-base.man > > index 9c786f298..4dc165f94 100644 > > --- a/lib/db-ctl-base.man > > +++ b/lib/db-ctl-base.man > > @@ -203,7 +203,7 @@ Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not > > exist. With \fB\-\-if-exists\fR, this command does nothing if > > \fIrecord\fR does not exist. > > . > > -.IP "[\fB\-\-id=@\fIname\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." > > +.IP "[\fB\-\-id=@\fIname\fR or \fB\-\-id=\fIuuid\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." > > I think, it should be [--id=(@name|uuid)] Ack. Done in v5. > > > Creates a new record in \fItable\fR and sets the initial values of > > each \fIcolumn\fR. Columns not explicitly set will receive their > > default values. Outputs the UUID of the new row. > > @@ -212,6 +212,9 @@ If \fB@\fIname\fR is specified, then the UUID for the new row may be > > referred to by that name elsewhere in the same \fB\*(PN\fR > > invocation in contexts where a UUID is expected. Such references may > > precede or follow the \fBcreate\fR command. > > +.IP > > +If a valid \fIuuid\fR is specified, then it is used as the UUID > > +of the new row. > > . > > .RS > > .IP "Caution (ovs-vsctl as example)" > > diff --git a/lib/db-ctl-base.xml b/lib/db-ctl-base.xml > > index f6efe98ea..4f63aa7f7 100644 > > --- a/lib/db-ctl-base.xml > > +++ b/lib/db-ctl-base.xml > > @@ -323,6 +323,10 @@ > > The argument line needs an update in this file as well. Ack. Done in v5. > > > invocation in contexts where a UUID is expected. Such references may > > precede or follow the <code>create</code> command. > > </p> > > + <p> > > + If a valid <var>uuid</var> is specified, then it is used as the > > + UUID of the new row. > > + </p> > > <dl> > > <dt>Caution (ovs-vsctl as example)</dt> > > <dd> > > diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > > index 8797686f9..332c4075b 100644 > > --- a/lib/ovsdb-idl-provider.h > > +++ b/lib/ovsdb-idl-provider.h > > @@ -74,6 +74,7 @@ struct ovsdb_idl_row { > > struct ovs_list dst_arcs; /* Backward arcs (ovsdb_idl_arc.dst_node). */ > > struct ovsdb_idl_table *table; /* Containing table. */ > > struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */ > > + bool persist_uuid; /* persist 'uuid' during insert txn if set. */ > > Comments should start with a capital letter. Ack. Done in v5. > > > bool parsed; /* Whether the row is parsed. */ > > struct ovs_list reparse_node; /* Rows that needs to be re-parsed due to > > * insertion of a referenced row. */ > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index 3b8741408..9ddb49273 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -2849,11 +2849,14 @@ substitute_uuids(struct json *json, const struct ovsdb_idl_txn *txn) > > > > row = ovsdb_idl_txn_get_row(txn, &uuid); > > if (row && !row->old_datum && row->new_datum) { > > - json_destroy(json); > > - > > - return json_array_create_2( > > - json_string_create("named-uuid"), > > - json_string_create_nocopy(ovsdb_data_row_name(&uuid))); > > + if (row->persist_uuid) { > > + return json; > > + } else { > > + json_destroy(json); > > + return json_array_create_2( > > + json_string_create("named-uuid"), > > + json_string_create_nocopy(ovsdb_data_row_name(&uuid))); > > + } > > } > > } > > > > @@ -3278,9 +3281,19 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) > > > > any_updates = true; > > > > - json_object_put(op, "uuid-name", > > - json_string_create_nocopy( > > - ovsdb_data_row_name(&row->uuid))); > > + char *uuid_json; > > + struct json *value; > > + if (row->persist_uuid) { > > + uuid_json = "uuid"; > > + value = json_string_create_nocopy( > > + xasprintf(UUID_FMT, UUID_ARGS(&row->uuid))); > > + } else { > > + uuid_json = "uuid-name"; > > + value = json_string_create_nocopy( > > + ovsdb_data_row_name(&row->uuid)); > > + } > > + > > + json_object_put(op, uuid_json, value); > > > > insert = xmalloc(sizeof *insert); > > insert->dummy = row->uuid; > > @@ -3765,6 +3778,32 @@ ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_) > > row->new_datum = NULL; > > } > > > > +static const struct ovsdb_idl_row * > > +ovsdb_idl_txn_insert__(struct ovsdb_idl_txn *txn, > > + const struct ovsdb_idl_table_class *class, > > + const struct uuid *uuid, > > + bool persist_uuid) > > +{ > > + struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class); > > + > > Should we also ovs_assert(uuid || !persist_uuid) ? Sounds good. Done in v5. Please take a look at v5 - https://patchwork.ozlabs.org/project/openvswitch/patch/20220718173703.296441-1-numans@ovn.org/ Numan > > > + if (uuid) { > > + ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); > > + row->uuid = *uuid; > > + row->persist_uuid = persist_uuid; > > + } else { > > + uuid_generate(&row->uuid); > > + row->persist_uuid = false; > > + } > > + > > + row->table = ovsdb_idl_table_from_class(txn->idl, class); > > + row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum); > > + hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); > > + hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); > > + ovsdb_idl_add_to_indexes(row); > > + > > + return row; > > +} > > + > > /* Inserts and returns a new row in the table with the specified 'class' in the > > * database with open transaction 'txn'. > > * > > @@ -3782,22 +3821,23 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn, > > const struct ovsdb_idl_table_class *class, > > const struct uuid *uuid) > > { > > - struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class); > > - > > - if (uuid) { > > - ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); > > - row->uuid = *uuid; > > - } else { > > - uuid_generate(&row->uuid); > > - } > > - > > - row->table = ovsdb_idl_table_from_class(txn->idl, class); > > - row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum); > > - hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); > > - hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); > > - ovsdb_idl_add_to_indexes(row); > > + return ovsdb_idl_txn_insert__(txn, class, uuid, false); > > +} > > > > - return row; > > +/* Inserts and returns a new row in the table with the specified 'class' in the > > + * database with open transaction 'txn'. > > + * > > + * The new row is assigned the specified UUID (which cannot be null). > > + * > > + * Usually this function is used indirectly through one of the > > + * "insert_persist_uuid" functions generated by ovsdb-idlc. */ > > +const struct ovsdb_idl_row * > > +ovsdb_idl_txn_insert_persist_uuid(struct ovsdb_idl_txn *txn, > > + const struct ovsdb_idl_table_class *class, > > + const struct uuid *uuid) > > +{ > > + ovs_assert(uuid); > > + return ovsdb_idl_txn_insert__(txn, class, uuid, true); > > } > > > > static void > > diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h > > index fbd9f671a..9a3e19f20 100644 > > --- a/lib/ovsdb-idl.h > > +++ b/lib/ovsdb-idl.h > > @@ -375,6 +375,9 @@ void ovsdb_idl_txn_delete(const struct ovsdb_idl_row *); > > const struct ovsdb_idl_row *ovsdb_idl_txn_insert( > > struct ovsdb_idl_txn *, const struct ovsdb_idl_table_class *, > > const struct uuid *); > > +const struct ovsdb_idl_row *ovsdb_idl_txn_insert_persist_uuid( > > + struct ovsdb_idl_txn *txn, const struct ovsdb_idl_table_class *class, > > + const struct uuid *uuid); > > > > struct ovsdb_idl *ovsdb_idl_txn_get_idl (struct ovsdb_idl_txn *); > > void ovsdb_idl_get_initial_snapshot(struct ovsdb_idl *); > > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in > > index 13c535939..7e09497ea 100755 > > --- a/ovsdb/ovsdb-idlc.in > > +++ b/ovsdb/ovsdb-idlc.in > > @@ -362,6 +362,8 @@ struct %(s)s *%(s)s_cursor_data(struct ovsdb_idl_cursor *); > > void %(s)s_init(struct %(s)s *); > > void %(s)s_delete(const struct %(s)s *); > > struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *); > > +struct %(s)s *%(s)s_insert_persist_uuid( > > + struct ovsdb_idl_txn *txn, const struct uuid *uuid); > > > > /* Returns true if the tracked column referenced by 'enum %(s)s_column_id' of > > * the row referenced by 'struct %(s)s *' was updated since the last change > > @@ -809,6 +811,19 @@ struct %(s)s * > > return %(s)s_cast(ovsdb_idl_txn_insert(txn, &%(p)stable_%(tl)s, NULL)); > > } > > > > +/* Inserts and returns a new row in the table "%(t)s" in the database > > + * with open transaction 'txn'. > > + * > > + * The new row is assigned the UUID specified in the 'uuid' parameter > > + * (which cannot be null). ovsdb-server will try to assign the same > > + * UUID when 'txn' is committed. */ > > +struct %(s)s * > > +%(s)s_insert_persist_uuid(struct ovsdb_idl_txn *txn, const struct uuid *uuid) > > +{ > > + return %(s)s_cast(ovsdb_idl_txn_insert_persist_uuid( > > + txn, &%(p)stable_%(tl)s, uuid)); > > +} > > + > > bool > > %(s)s_is_updated(const struct %(s)s *row, enum %(s)s_column_id column) > > { > > diff --git a/python/TODO.rst b/python/TODO.rst > > index 3a53489f1..a0b3e807a 100644 > > --- a/python/TODO.rst > > +++ b/python/TODO.rst > > @@ -32,3 +32,5 @@ Python Bindings To-do List > > > > * Support write-only-changed monitor mode (equivalent of > > OVSDB_IDL_WRITE_CHANGED_ONLY). > > + > > + * Support accepting the uuid for row inserts. > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > > index d6cd2c084..abf4fb9cf 100644 > > --- a/tests/ovs-vsctl.at > > +++ b/tests/ovs-vsctl.at > > @@ -1710,3 +1710,28 @@ ingress_policing_kpkts_rate: 100 > > ]) > > OVS_VSCTL_CLEANUP > > AT_CLEANUP > > + > > +AT_SETUP([ovs-vsctl create bridge with uuid]) > > +AT_KEYWORDS([create bridge with uuid]) > > +OVS_VSCTL_SETUP > > + > > +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 create bridge \ > > +name=tst0 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [0],[dnl > > +c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > > +]) > > + > > +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 create bridge \ > > +name=tst1 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [1], [ignore], [ignore]) > > + > > +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge], [0], [dnl > > +c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > > +tst0 > > +]) > > + > > +ovs-vsctl --no-wait --id=@a create bridge \ > > +name=tst1 -- add open . bridges @a > > + > > +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0], [ignore]) > > + > > +OVS_VSCTL_CLEANUP > > +AT_CLEANUP > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > > index d1cfa59c5..28c032105 100644 > > --- a/tests/ovsdb-idl.at > > +++ b/tests/ovsdb-idl.at > > @@ -2466,3 +2466,30 @@ unix:socket2 remote has col id in table simple7 > > > > OVSDB_SERVER_SHUTDOWN > > AT_CLEANUP > > + > > +AT_SETUP([idl creating rows with persistent uuid - C]) > > +AT_KEYWORDS([ovsdb client idl txn]) > > + > > +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest.ovsschema"]) > > +AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl-txn-persistent-uuid unix:socket], > > + [0], [stdout], [stderr]) > > +AT_CHECK([sort stdout], [0], > > + [[000: After inserting simple, simple3 and simple4 > > +001: table simple3: name=simple3 uset=[] uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222 > > +001: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > > +001: table simple: i=0 r=0 b=false s=simple u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333 > > +002: After inserting simple with same uuid > > +003: table simple3: name=simple3 uset=[] uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222 > > +003: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 > > +003: table simple: i=0 r=0 b=false s=simple u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333 > > +004: End test > > +]]) > > + > > +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl > > +test-ovsdb|ovsdb_idl|transaction error: {"details":"This UUID would dnl > > +duplicate a UUID already present within the table or deleted within dnl > > +the same transaction.","error":"duplicate uuid","syntax":"\"c5cc12f8-eaa1-43a7-8a73-bccd18df3333\""} > > +]) > > + > > +OVSDB_SERVER_SHUTDOWN > > +AT_CLEANUP > > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c > > index 0fef1f978..515a3015d 100644 > > --- a/tests/test-ovsdb.c > > +++ b/tests/test-ovsdb.c > > @@ -3389,6 +3389,63 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx) > > ovsdb_idl_destroy(idl); > > } > > > > +static void > > +do_idl_txn_persistent_uuid(struct ovs_cmdl_context *ctx) > > +{ > > + struct ovsdb_idl *idl; > > + struct ovsdb_idl_txn *myTxn; > > + int step = 0; > > + > > + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); > > + ovsdb_idl_get_initial_snapshot(idl); > > + ovsdb_idl_run(idl); > > + > > + myTxn = ovsdb_idl_txn_create(idl); > > + > > + struct uuid uuid1; > > + uuid_from_string(&uuid1, "c5cc12f8-eaa1-43a7-8a73-bccd18df1111"); > > + > > + struct uuid uuid2; > > + uuid_from_string(&uuid2, "c5cc12f8-eaa1-43a7-8a73-bccd18df2222"); > > + > > + struct idltest_simple4 *simple4_row = > > + idltest_simple4_insert_persist_uuid(myTxn, &uuid1); > > + idltest_simple4_set_name(simple4_row, "simple4"); > > + > > + struct idltest_simple3 *simple3_row = > > + idltest_simple3_insert_persist_uuid(myTxn, &uuid2); > > + idltest_simple3_set_name(simple3_row, "simple3"); > > + idltest_simple3_set_uref(simple3_row, &simple4_row, 1); > > + > > + struct uuid uuid3; > > + uuid_from_string(&uuid3, "c5cc12f8-eaa1-43a7-8a73-bccd18df3333"); > > + > > + struct idltest_simple *simple_row = > > + idltest_simple_insert_persist_uuid(myTxn, &uuid3); > > + idltest_simple_set_s(simple_row, "simple"); > > + > > + ovsdb_idl_txn_commit_block(myTxn); > > + ovsdb_idl_txn_destroy(myTxn); > > + ovsdb_idl_get_initial_snapshot(idl); > > + printf("%03d: After inserting simple, simple3 and simple4\n", step++); > > + print_idl(idl, step++, false); > > + > > + /* Create another txn, insert the row in simple table with the existing > > + * uuid. */ > > + myTxn = ovsdb_idl_txn_create(idl); > > + simple_row = > > + idltest_simple_insert_persist_uuid(myTxn, &uuid3); > > + idltest_simple_set_s(simple_row, "simple_foo"); > > + ovsdb_idl_txn_commit_block(myTxn); > > + ovsdb_idl_txn_destroy(myTxn); > > + ovsdb_idl_get_initial_snapshot(idl); > > + printf("%03d: After inserting simple with same uuid\n", step++); > > + print_idl(idl, step++, false); > > + > > + ovsdb_idl_destroy(idl); > > + printf("%03d: End test\n", step++); > > +} > > + > > static struct ovs_cmdl_command all_commands[] = { > > { "log-io", NULL, 2, INT_MAX, do_log_io, OVS_RO }, > > { "default-atoms", NULL, 0, 0, do_default_atoms, OVS_RO }, > > @@ -3429,6 +3486,8 @@ static struct ovs_cmdl_command all_commands[] = { > > do_idl_partial_update_set_column, OVS_RO }, > > { "idl-table-column-check", NULL, 2, 2, > > do_idl_table_column_check, OVS_RO }, > > + { "idl-txn-persistent-uuid", NULL, 1, INT_MAX, > > + do_idl_txn_persistent_uuid, OVS_RO }, > > { "help", NULL, 0, INT_MAX, do_help, OVS_RO }, > > { NULL, NULL, 0, 0, NULL, OVS_RO }, > > }; > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c index 707456158..f39e090a0 100644 --- a/lib/db-ctl-base.c +++ b/lib/db-ctl-base.c @@ -1732,28 +1732,42 @@ cmd_create(struct ctl_context *ctx) const struct ovsdb_idl_row *row; const struct uuid *uuid = NULL; int i; + struct uuid uuid_; + bool persist_uuid = false; ctx->error = get_table(table_name, &table); if (ctx->error) { return; } + if (id) { - struct ovsdb_symbol *symbol = NULL; + if (uuid_from_string(&uuid_, id)) { + uuid = &uuid_; + persist_uuid = true; + } else { + struct ovsdb_symbol *symbol = NULL; - ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL); - if (ctx->error) { - return; - } - if (table->is_root) { - /* This table is in the root set, meaning that rows created in it - * won't disappear even if they are unreferenced, so disable - * warnings about that by pretending that there is a reference. */ - symbol->strong_ref = true; + ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL); + if (ctx->error) { + return; + } + if (table->is_root) { + /* This table is in the root set, meaning that rows created in + * it won't disappear even if they are unreferenced, so disable + * warnings about that by pretending that there is a + * reference. */ + symbol->strong_ref = true; + } + uuid = &symbol->uuid; } - uuid = &symbol->uuid; } - row = ovsdb_idl_txn_insert(ctx->txn, table, uuid); + if (persist_uuid) { + row = ovsdb_idl_txn_insert_persist_uuid(ctx->txn, table, uuid); + } else { + row = ovsdb_idl_txn_insert(ctx->txn, table, uuid); + } + for (i = 2; i < ctx->argc; i++) { ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab); if (ctx->error) { diff --git a/lib/db-ctl-base.man b/lib/db-ctl-base.man index 9c786f298..4dc165f94 100644 --- a/lib/db-ctl-base.man +++ b/lib/db-ctl-base.man @@ -203,7 +203,7 @@ Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR does not exist. With \fB\-\-if-exists\fR, this command does nothing if \fIrecord\fR does not exist. . -.IP "[\fB\-\-id=@\fIname\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." +.IP "[\fB\-\-id=@\fIname\fR or \fB\-\-id=\fIuuid\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..." Creates a new record in \fItable\fR and sets the initial values of each \fIcolumn\fR. Columns not explicitly set will receive their default values. Outputs the UUID of the new row. @@ -212,6 +212,9 @@ If \fB@\fIname\fR is specified, then the UUID for the new row may be referred to by that name elsewhere in the same \fB\*(PN\fR invocation in contexts where a UUID is expected. Such references may precede or follow the \fBcreate\fR command. +.IP +If a valid \fIuuid\fR is specified, then it is used as the UUID +of the new row. . .RS .IP "Caution (ovs-vsctl as example)" diff --git a/lib/db-ctl-base.xml b/lib/db-ctl-base.xml index f6efe98ea..4f63aa7f7 100644 --- a/lib/db-ctl-base.xml +++ b/lib/db-ctl-base.xml @@ -323,6 +323,10 @@ invocation in contexts where a UUID is expected. Such references may precede or follow the <code>create</code> command. </p> + <p> + If a valid <var>uuid</var> is specified, then it is used as the + UUID of the new row. + </p> <dl> <dt>Caution (ovs-vsctl as example)</dt> <dd> diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h index 8797686f9..332c4075b 100644 --- a/lib/ovsdb-idl-provider.h +++ b/lib/ovsdb-idl-provider.h @@ -74,6 +74,7 @@ struct ovsdb_idl_row { struct ovs_list dst_arcs; /* Backward arcs (ovsdb_idl_arc.dst_node). */ struct ovsdb_idl_table *table; /* Containing table. */ struct ovsdb_datum *old_datum; /* Committed data (null if orphaned). */ + bool persist_uuid; /* persist 'uuid' during insert txn if set. */ bool parsed; /* Whether the row is parsed. */ struct ovs_list reparse_node; /* Rows that needs to be re-parsed due to * insertion of a referenced row. */ diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 3b8741408..9ddb49273 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -2849,11 +2849,14 @@ substitute_uuids(struct json *json, const struct ovsdb_idl_txn *txn) row = ovsdb_idl_txn_get_row(txn, &uuid); if (row && !row->old_datum && row->new_datum) { - json_destroy(json); - - return json_array_create_2( - json_string_create("named-uuid"), - json_string_create_nocopy(ovsdb_data_row_name(&uuid))); + if (row->persist_uuid) { + return json; + } else { + json_destroy(json); + return json_array_create_2( + json_string_create("named-uuid"), + json_string_create_nocopy(ovsdb_data_row_name(&uuid))); + } } } @@ -3278,9 +3281,19 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn) any_updates = true; - json_object_put(op, "uuid-name", - json_string_create_nocopy( - ovsdb_data_row_name(&row->uuid))); + char *uuid_json; + struct json *value; + if (row->persist_uuid) { + uuid_json = "uuid"; + value = json_string_create_nocopy( + xasprintf(UUID_FMT, UUID_ARGS(&row->uuid))); + } else { + uuid_json = "uuid-name"; + value = json_string_create_nocopy( + ovsdb_data_row_name(&row->uuid)); + } + + json_object_put(op, uuid_json, value); insert = xmalloc(sizeof *insert); insert->dummy = row->uuid; @@ -3765,6 +3778,32 @@ ovsdb_idl_txn_delete(const struct ovsdb_idl_row *row_) row->new_datum = NULL; } +static const struct ovsdb_idl_row * +ovsdb_idl_txn_insert__(struct ovsdb_idl_txn *txn, + const struct ovsdb_idl_table_class *class, + const struct uuid *uuid, + bool persist_uuid) +{ + struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class); + + if (uuid) { + ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); + row->uuid = *uuid; + row->persist_uuid = persist_uuid; + } else { + uuid_generate(&row->uuid); + row->persist_uuid = false; + } + + row->table = ovsdb_idl_table_from_class(txn->idl, class); + row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum); + hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); + hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); + ovsdb_idl_add_to_indexes(row); + + return row; +} + /* Inserts and returns a new row in the table with the specified 'class' in the * database with open transaction 'txn'. * @@ -3782,22 +3821,23 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn, const struct ovsdb_idl_table_class *class, const struct uuid *uuid) { - struct ovsdb_idl_row *row = ovsdb_idl_row_create__(class); - - if (uuid) { - ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); - row->uuid = *uuid; - } else { - uuid_generate(&row->uuid); - } - - row->table = ovsdb_idl_table_from_class(txn->idl, class); - row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum); - hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid)); - hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid)); - ovsdb_idl_add_to_indexes(row); + return ovsdb_idl_txn_insert__(txn, class, uuid, false); +} - return row; +/* Inserts and returns a new row in the table with the specified 'class' in the + * database with open transaction 'txn'. + * + * The new row is assigned the specified UUID (which cannot be null). + * + * Usually this function is used indirectly through one of the + * "insert_persist_uuid" functions generated by ovsdb-idlc. */ +const struct ovsdb_idl_row * +ovsdb_idl_txn_insert_persist_uuid(struct ovsdb_idl_txn *txn, + const struct ovsdb_idl_table_class *class, + const struct uuid *uuid) +{ + ovs_assert(uuid); + return ovsdb_idl_txn_insert__(txn, class, uuid, true); } static void diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h index fbd9f671a..9a3e19f20 100644 --- a/lib/ovsdb-idl.h +++ b/lib/ovsdb-idl.h @@ -375,6 +375,9 @@ void ovsdb_idl_txn_delete(const struct ovsdb_idl_row *); const struct ovsdb_idl_row *ovsdb_idl_txn_insert( struct ovsdb_idl_txn *, const struct ovsdb_idl_table_class *, const struct uuid *); +const struct ovsdb_idl_row *ovsdb_idl_txn_insert_persist_uuid( + struct ovsdb_idl_txn *txn, const struct ovsdb_idl_table_class *class, + const struct uuid *uuid); struct ovsdb_idl *ovsdb_idl_txn_get_idl (struct ovsdb_idl_txn *); void ovsdb_idl_get_initial_snapshot(struct ovsdb_idl *); diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index 13c535939..7e09497ea 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -362,6 +362,8 @@ struct %(s)s *%(s)s_cursor_data(struct ovsdb_idl_cursor *); void %(s)s_init(struct %(s)s *); void %(s)s_delete(const struct %(s)s *); struct %(s)s *%(s)s_insert(struct ovsdb_idl_txn *); +struct %(s)s *%(s)s_insert_persist_uuid( + struct ovsdb_idl_txn *txn, const struct uuid *uuid); /* Returns true if the tracked column referenced by 'enum %(s)s_column_id' of * the row referenced by 'struct %(s)s *' was updated since the last change @@ -809,6 +811,19 @@ struct %(s)s * return %(s)s_cast(ovsdb_idl_txn_insert(txn, &%(p)stable_%(tl)s, NULL)); } +/* Inserts and returns a new row in the table "%(t)s" in the database + * with open transaction 'txn'. + * + * The new row is assigned the UUID specified in the 'uuid' parameter + * (which cannot be null). ovsdb-server will try to assign the same + * UUID when 'txn' is committed. */ +struct %(s)s * +%(s)s_insert_persist_uuid(struct ovsdb_idl_txn *txn, const struct uuid *uuid) +{ + return %(s)s_cast(ovsdb_idl_txn_insert_persist_uuid( + txn, &%(p)stable_%(tl)s, uuid)); +} + bool %(s)s_is_updated(const struct %(s)s *row, enum %(s)s_column_id column) { diff --git a/python/TODO.rst b/python/TODO.rst index 3a53489f1..a0b3e807a 100644 --- a/python/TODO.rst +++ b/python/TODO.rst @@ -32,3 +32,5 @@ Python Bindings To-do List * Support write-only-changed monitor mode (equivalent of OVSDB_IDL_WRITE_CHANGED_ONLY). + + * Support accepting the uuid for row inserts. diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at index d6cd2c084..abf4fb9cf 100644 --- a/tests/ovs-vsctl.at +++ b/tests/ovs-vsctl.at @@ -1710,3 +1710,28 @@ ingress_policing_kpkts_rate: 100 ]) OVS_VSCTL_CLEANUP AT_CLEANUP + +AT_SETUP([ovs-vsctl create bridge with uuid]) +AT_KEYWORDS([create bridge with uuid]) +OVS_VSCTL_SETUP + +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 create bridge \ +name=tst0 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [0],[dnl +c5cc12f8-eaa1-43a7-8a73-bccd18df1111 +]) + +AT_CHECK([ovs-vsctl --no-wait --id=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 create bridge \ +name=tst1 -- add open . bridges c5cc12f8-eaa1-43a7-8a73-bccd18df1111], [1], [ignore], [ignore]) + +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge], [0], [dnl +c5cc12f8-eaa1-43a7-8a73-bccd18df1111 +tst0 +]) + +ovs-vsctl --no-wait --id=@a create bridge \ +name=tst1 -- add open . bridges @a + +AT_CHECK([ovs-vsctl --no-wait --bare --columns _uuid,name list bridge tst1], [0], [ignore]) + +OVS_VSCTL_CLEANUP +AT_CLEANUP diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index d1cfa59c5..28c032105 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -2466,3 +2466,30 @@ unix:socket2 remote has col id in table simple7 OVSDB_SERVER_SHUTDOWN AT_CLEANUP + +AT_SETUP([idl creating rows with persistent uuid - C]) +AT_KEYWORDS([ovsdb client idl txn]) + +AT_CHECK([ovsdb_start_idltest "" "$abs_srcdir/idltest.ovsschema"]) +AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl-txn-persistent-uuid unix:socket], + [0], [stdout], [stderr]) +AT_CHECK([sort stdout], [0], + [[000: After inserting simple, simple3 and simple4 +001: table simple3: name=simple3 uset=[] uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222 +001: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 +001: table simple: i=0 r=0 b=false s=simple u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333 +002: After inserting simple with same uuid +003: table simple3: name=simple3 uset=[] uref=[c5cc12f8-eaa1-43a7-8a73-bccd18df1111] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df2222 +003: table simple4: name=simple4 uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df1111 +003: table simple: i=0 r=0 b=false s=simple u=00000000-0000-0000-0000-000000000000 ia=[] ra=[] ba=[] sa=[] ua=[] uuid=c5cc12f8-eaa1-43a7-8a73-bccd18df3333 +004: End test +]]) + +AT_CHECK([grep ovsdb_idl stderr | sort], [0], [dnl +test-ovsdb|ovsdb_idl|transaction error: {"details":"This UUID would dnl +duplicate a UUID already present within the table or deleted within dnl +the same transaction.","error":"duplicate uuid","syntax":"\"c5cc12f8-eaa1-43a7-8a73-bccd18df3333\""} +]) + +OVSDB_SERVER_SHUTDOWN +AT_CLEANUP diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c index 0fef1f978..515a3015d 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -3389,6 +3389,63 @@ do_idl_table_column_check(struct ovs_cmdl_context *ctx) ovsdb_idl_destroy(idl); } +static void +do_idl_txn_persistent_uuid(struct ovs_cmdl_context *ctx) +{ + struct ovsdb_idl *idl; + struct ovsdb_idl_txn *myTxn; + int step = 0; + + idl = ovsdb_idl_create(ctx->argv[1], &idltest_idl_class, true, true); + ovsdb_idl_get_initial_snapshot(idl); + ovsdb_idl_run(idl); + + myTxn = ovsdb_idl_txn_create(idl); + + struct uuid uuid1; + uuid_from_string(&uuid1, "c5cc12f8-eaa1-43a7-8a73-bccd18df1111"); + + struct uuid uuid2; + uuid_from_string(&uuid2, "c5cc12f8-eaa1-43a7-8a73-bccd18df2222"); + + struct idltest_simple4 *simple4_row = + idltest_simple4_insert_persist_uuid(myTxn, &uuid1); + idltest_simple4_set_name(simple4_row, "simple4"); + + struct idltest_simple3 *simple3_row = + idltest_simple3_insert_persist_uuid(myTxn, &uuid2); + idltest_simple3_set_name(simple3_row, "simple3"); + idltest_simple3_set_uref(simple3_row, &simple4_row, 1); + + struct uuid uuid3; + uuid_from_string(&uuid3, "c5cc12f8-eaa1-43a7-8a73-bccd18df3333"); + + struct idltest_simple *simple_row = + idltest_simple_insert_persist_uuid(myTxn, &uuid3); + idltest_simple_set_s(simple_row, "simple"); + + ovsdb_idl_txn_commit_block(myTxn); + ovsdb_idl_txn_destroy(myTxn); + ovsdb_idl_get_initial_snapshot(idl); + printf("%03d: After inserting simple, simple3 and simple4\n", step++); + print_idl(idl, step++, false); + + /* Create another txn, insert the row in simple table with the existing + * uuid. */ + myTxn = ovsdb_idl_txn_create(idl); + simple_row = + idltest_simple_insert_persist_uuid(myTxn, &uuid3); + idltest_simple_set_s(simple_row, "simple_foo"); + ovsdb_idl_txn_commit_block(myTxn); + ovsdb_idl_txn_destroy(myTxn); + ovsdb_idl_get_initial_snapshot(idl); + printf("%03d: After inserting simple with same uuid\n", step++); + print_idl(idl, step++, false); + + ovsdb_idl_destroy(idl); + printf("%03d: End test\n", step++); +} + static struct ovs_cmdl_command all_commands[] = { { "log-io", NULL, 2, INT_MAX, do_log_io, OVS_RO }, { "default-atoms", NULL, 0, 0, do_default_atoms, OVS_RO }, @@ -3429,6 +3486,8 @@ static struct ovs_cmdl_command all_commands[] = { do_idl_partial_update_set_column, OVS_RO }, { "idl-table-column-check", NULL, 2, 2, do_idl_table_column_check, OVS_RO }, + { "idl-txn-persistent-uuid", NULL, 1, INT_MAX, + do_idl_txn_persistent_uuid, OVS_RO }, { "help", NULL, 0, INT_MAX, do_help, OVS_RO }, { NULL, NULL, 0, 0, NULL, OVS_RO }, };