| Message ID | 20220926175259.703169-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v7] 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 | success | github build: passed |
| 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 108 characters long (recommended limit is 79)
#119 FILE: lib/db-ctl-base.man:206:
.IP "[\fB\-\-id=(@\fIname\fR | \fIuuid\fR] \fBcreate\fR \fItable column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
WARNING: Line is 174 characters long (recommended limit is 79)
#142 FILE: lib/db-ctl-base.xml:313:
<dt>[<code>--id=(@</code><var>name</var>|<var>uuid</var>)] <code>create</code> <var>table column</var>[<code>:</code><var>key</var>]<code>=</code><var>value</var>...</dt>
Lines checked: 492, Warnings: 2, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 9/26/22 19:52, 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> > --- > v6 -> v7 > --- > * Rebased to resolve conflicts. > > v5 -> v6 > --- > * Rebased to resolve conflicts. > > v4 -> v5 > --- > * Addressed review comments from Ilya. > - Added NEWS item entry. > > 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. > > > NEWS | 2 + > lib/db-ctl-base.c | 38 ++++++++++++------ > lib/db-ctl-base.man | 5 ++- > lib/db-ctl-base.xml | 6 ++- > lib/ovsdb-idl-provider.h | 1 + > lib/ovsdb-idl.c | 85 +++++++++++++++++++++++++++++----------- > 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 ++++++++++++++++++++++++++++ > 12 files changed, 231 insertions(+), 37 deletions(-) > > 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. How hard it is to actually add the support? it would be great to not increase the difference between implementations. Sometimes we do not add features to python idl, but that mostly happens when they depend on some other missing features. UUID persistense seems to not have any extra dependencies. > 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 965b0f913..3edb2bc78 100644 > --- a/tests/test-ovsdb.c > +++ b/tests/test-ovsdb.c > @@ -3451,6 +3451,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++); > +} This test case is a bit worrying, because whenever we'll have a python implementation, the test case will have to be duplicated or fully re-written. Can we add a new command to the 'transact' test instead, e.g. 'insert-with-uuid' and write a generic test case for it? > + > 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 }, > @@ -3491,6 +3548,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, Oct 25, 2022 at 5:17 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 9/26/22 19:52, 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> > > --- > > v6 -> v7 > > --- > > * Rebased to resolve conflicts. > > > > v5 -> v6 > > --- > > * Rebased to resolve conflicts. > > > > v4 -> v5 > > --- > > * Addressed review comments from Ilya. > > - Added NEWS item entry. > > > > 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. > > > > > > NEWS | 2 + > > lib/db-ctl-base.c | 38 ++++++++++++------ > > lib/db-ctl-base.man | 5 ++- > > lib/db-ctl-base.xml | 6 ++- > > lib/ovsdb-idl-provider.h | 1 + > > lib/ovsdb-idl.c | 85 +++++++++++++++++++++++++++++----------- > > 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 ++++++++++++++++++++++++++++ > > 12 files changed, 231 insertions(+), 37 deletions(-) > > > > 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. > Thanks for the review. Sorry for the late reply. > How hard it is to actually add the support? > it would be great to not increase the difference between > implementations. Sometimes we do not add features to > python idl, but that mostly happens when they depend on > some other missing features. UUID persistense seems to > not have any extra dependencies. > It turned out to be straightforward. Addressed in v8. > > 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 965b0f913..3edb2bc78 100644 > > --- a/tests/test-ovsdb.c > > +++ b/tests/test-ovsdb.c > > @@ -3451,6 +3451,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++); > > +} > > This test case is a bit worrying, because whenever we'll have > a python implementation, the test case will have to be > duplicated or fully re-written. Can we add a new command > to the 'transact' test instead, e.g. 'insert-with-uuid' and > write a generic test case for it? Addressed this in v8. It was not possible to add a new comment in the transact tests (I suppose you mean tests in ovsdb-transaction.at) as the tests there don't create any IDL objects. But added subcommit - "insert_with_uuid" in do_idl set command. Please take a look at v8 - https://patchwork.ozlabs.org/project/openvswitch/patch/20221128035613.36207-1-numans@ovn.org/ @Terry Wilson Can you please take a look at v8 of this patch as it also modifies the python idl. I want to make sure this change doesn't break ovsdbapp. Thanks Numan > > > + > > 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 }, > > @@ -3491,6 +3548,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/NEWS b/NEWS index d5ec09813..d0c8e5f95 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,8 @@ Post-v3.0.0 - Windows: * Conntrack IPv6 fragment support. + - OVSDB-IDL: + * Add the support to specify the uuid for row insert. v3.0.0 - 15 Aug 2022 -------------------- diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c index bc85e9921..856832a04 100644 --- a/lib/db-ctl-base.c +++ b/lib/db-ctl-base.c @@ -1731,29 +1731,43 @@ cmd_create(struct ctl_context *ctx) const struct ovsdb_idl_table_class *table; const struct ovsdb_idl_row *row; const struct uuid *uuid = NULL; + bool persist_uuid = false; + struct uuid uuid_; int i; 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 a529d8b4d..c8111c9ef 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 | \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..27c999fe7 100644 --- a/lib/db-ctl-base.xml +++ b/lib/db-ctl-base.xml @@ -310,7 +310,7 @@ </p> </dd> - <dt>[<code>--id=@</code><var>name</var>] <code>create</code> <var>table column</var>[<code>:</code><var>key</var>]<code>=</code><var>value</var>...</dt> + <dt>[<code>--id=(@</code><var>name</var>|<var>uuid</var>)] <code>create</code> <var>table column</var>[<code>:</code><var>key</var>]<code>=</code><var>value</var>...</dt> <dd> <p> Creates a new record in <var>table</var> and sets the initial values of @@ -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..8d2b7d6b9 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 576979f9e..9a10d91e4 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -2851,11 +2851,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))); + } } } @@ -3280,9 +3283,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; @@ -3766,6 +3779,31 @@ 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); + + ovs_assert(uuid || !persist_uuid); + if (uuid) { + ovs_assert(!ovsdb_idl_txn_get_row(txn, uuid)); + row->uuid = *uuid; + } else { + uuid_generate(&row->uuid); + } + row->persist_uuid = persist_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 row; +} + /* Inserts and returns a new row in the table with the specified 'class' in the * database with open transaction 'txn'. * @@ -3783,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 5a97a8ea3..9a54f06a1 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 965b0f913..3edb2bc78 100644 --- a/tests/test-ovsdb.c +++ b/tests/test-ovsdb.c @@ -3451,6 +3451,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 }, @@ -3491,6 +3548,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 }, };