Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.
> -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Friday, September 16, 2016 3:51 AM > To: Alin Serdean > Cc: Andy Zhou ; > > Subject: Re: [ovs-dev] [PATCH] replication: Be more careful about JSON > parsing and simplify code. > > Is it intermittent, then? [Alin Serdean] 7/10 runs pass ratio > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.
Is it intermittent, then? On Fri, Sep 16, 2016 at 12:47:14AM +, Alin Serdean wrote: > Sorry to bump on this problem again, but the issue reproduces from time to > time while testing under windows. > > Applying (http://openvswitch.org/pipermail/dev/2016-September/079315.html) > would fix the symptom. > > STACK_TEXT: > 00c0`e63cf680 7ff7`f9306607 : 0264`7e439f00 ` > 0264`7e42def8 `2b038801 : > ovsdb_server!ovsdb_datum_compare_3way+0x1d > 00c0`e63cf6c0 7ff7`f92df779 : 0264`7e439f00 ` > 0264`7e42def8 7ff7`f92df5c5 : ovsdb_server!ovsdb_datum_equals+0x27 > 00c0`e63cf700 7ff7`f92e1034 : 0264`7e43fb80 0264`7e439ea0 > ` 0264`7e4395c0 : > ovsdb_server!update_monitor_row_data+0xa9 > 00c0`e63cf760 7ff7`f92e136d : ` 0264`7e439ea0 > 0264`7e43fb80 0264`7e4395c0 : > ovsdb_server!ovsdb_monitor_changes_update+0x114 > 00c0`e63cf7c0 7ff7`f92e6e60 : ` 0264`7e439ea0 > 0264`7e4394e4 00c0`e63cf8b0 : > ovsdb_server!ovsdb_monitor_change_cb+0x1dd > 00c0`e63cf840 7ff7`f92e178c : 0264`7e43ca90 7ff7`f92e1190 > 00c0`e63cf8b0 0264`7e439c80 : > ovsdb_server!ovsdb_txn_for_each_change+0x130 > 00c0`e63cf880 7ff7`f92ea7b9 : 0264`7e43fab0 0264`7e43ca90 > ` `0001 : ovsdb_server!ovsdb_monitor_commit+0x5c > 00c0`e63cf8e0 7ff7`f92e6aac : 0264`7e43ca90 ` > 8f4644b3`452879f6 6eb99859`8e0f9f99 : ovsdb_server!ovsdb_txn_commit_+0x259 > 00c0`e63cf930 7ff7`f92e4a51 : 0264`7e43ca90 0264`7e43e500 > 0264`7e438de0 0264`7e43ca90 : ovsdb_server!ovsdb_txn_commit+0x1c > 00c0`e63cf970 7ff7`f92e36ac : 0264`7e43e3f0 0264`7e438de0 > 0264`7e43e5d0 0264`7e43e5d0 : ovsdb_server!process_notification+0x131 > 00c0`e63cf9d0 7ff7`f92d3199 : 0264`7e42dcf0 00c0`e63cfb38 > 00c0`e63cfcd0 7ff7`f92ff89f : ovsdb_server!replication_run+0x23c > 00c0`e63cfae0 7ff7`f92d3db3 : 0264`7e42dcf0 00c0`e63cfc48 > 0264`7e42fdd0 00c0`e63cfcd0 : ovsdb_server!main_loop+0x209 > 00c0`e63cfb70 7ff7`f9337f3c : 7ff7`0001 0264`7e426350 > ` ` : ovsdb_server!main+0x993 > 00c0`e63cfd30 7ff7`f933807e : ` ` > ` ` : ovsdb_server!__tmainCRTStartup+0xec > 00c0`e63cfd80 7ffd`5b728102 : ` ` > ` ` : ovsdb_server!mainCRTStartup+0xe > 00c0`e63cfdb0 7ffd`5dc5c5b4 : 7ffd`5b7280e0 ` > ` ` : kernel32!BaseThreadInitThunk+0x22 > 00c0`e63cfde0 ` : ` ` > ` ` : ntdll!RtlUserThreadStart+0x34 > > > STACK_COMMAND: ~0s; .ecxr ; kb > > FAULTING_SOURCE_LINE: ovsdb-data.c > > FAULTING_SOURCE_FILE: ovsdb-data.c > > FAULTING_SOURCE_LINE_NUMBER: 1626 > > FAULTING_SOURCE_CODE: > 1622: const struct ovsdb_type *type) > 1623: { > 1624: int cmp; > 1625: > > 1626: if (a->n != b->n) { > 1627: return a->n < b->n ? -1 : 1; > 1628: } > 1629: > 1630: cmp = atom_arrays_compare_3way(a->keys, b->keys, type->key.type, > a->n); > 1631: if (cmp) { > > > > > I don't know the exact cause of the problem, but this new > > > > implementation leaves me more confident due to its simplicity. > > > > > > > > Reported-by: Joe Stringer > > > > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/ > > > > 079315.html > > > > Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements") > > > > Signed-off-by: Ben Pfaff > > > > > > > > > > Acked-by: Andy Zhou > > > > > > Thanks for tracking this down the issue and for the simplification. > > > Sorry for not jumpping on Joe's original report sonner. I did not have > > > internet access from my laptop for the last few days. > > > > You're on vacation, no worries. > > > > Applied to master and branch-2.6, thanks! > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.
Sorry to bump on this problem again, but the issue reproduces from time to time while testing under windows. Applying (http://openvswitch.org/pipermail/dev/2016-September/079315.html) would fix the symptom. STACK_TEXT: 00c0`e63cf680 7ff7`f9306607 : 0264`7e439f00 ` 0264`7e42def8 `2b038801 : ovsdb_server!ovsdb_datum_compare_3way+0x1d 00c0`e63cf6c0 7ff7`f92df779 : 0264`7e439f00 ` 0264`7e42def8 7ff7`f92df5c5 : ovsdb_server!ovsdb_datum_equals+0x27 00c0`e63cf700 7ff7`f92e1034 : 0264`7e43fb80 0264`7e439ea0 ` 0264`7e4395c0 : ovsdb_server!update_monitor_row_data+0xa9 00c0`e63cf760 7ff7`f92e136d : ` 0264`7e439ea0 0264`7e43fb80 0264`7e4395c0 : ovsdb_server!ovsdb_monitor_changes_update+0x114 00c0`e63cf7c0 7ff7`f92e6e60 : ` 0264`7e439ea0 0264`7e4394e4 00c0`e63cf8b0 : ovsdb_server!ovsdb_monitor_change_cb+0x1dd 00c0`e63cf840 7ff7`f92e178c : 0264`7e43ca90 7ff7`f92e1190 00c0`e63cf8b0 0264`7e439c80 : ovsdb_server!ovsdb_txn_for_each_change+0x130 00c0`e63cf880 7ff7`f92ea7b9 : 0264`7e43fab0 0264`7e43ca90 ` `0001 : ovsdb_server!ovsdb_monitor_commit+0x5c 00c0`e63cf8e0 7ff7`f92e6aac : 0264`7e43ca90 ` 8f4644b3`452879f6 6eb99859`8e0f9f99 : ovsdb_server!ovsdb_txn_commit_+0x259 00c0`e63cf930 7ff7`f92e4a51 : 0264`7e43ca90 0264`7e43e500 0264`7e438de0 0264`7e43ca90 : ovsdb_server!ovsdb_txn_commit+0x1c 00c0`e63cf970 7ff7`f92e36ac : 0264`7e43e3f0 0264`7e438de0 0264`7e43e5d0 0264`7e43e5d0 : ovsdb_server!process_notification+0x131 00c0`e63cf9d0 7ff7`f92d3199 : 0264`7e42dcf0 00c0`e63cfb38 00c0`e63cfcd0 7ff7`f92ff89f : ovsdb_server!replication_run+0x23c 00c0`e63cfae0 7ff7`f92d3db3 : 0264`7e42dcf0 00c0`e63cfc48 0264`7e42fdd0 00c0`e63cfcd0 : ovsdb_server!main_loop+0x209 00c0`e63cfb70 7ff7`f9337f3c : 7ff7`0001 0264`7e426350 ` ` : ovsdb_server!main+0x993 00c0`e63cfd30 7ff7`f933807e : ` ` ` ` : ovsdb_server!__tmainCRTStartup+0xec 00c0`e63cfd80 7ffd`5b728102 : ` ` ` ` : ovsdb_server!mainCRTStartup+0xe 00c0`e63cfdb0 7ffd`5dc5c5b4 : 7ffd`5b7280e0 ` ` ` : kernel32!BaseThreadInitThunk+0x22 00c0`e63cfde0 ` : ` ` ` ` : ntdll!RtlUserThreadStart+0x34 STACK_COMMAND: ~0s; .ecxr ; kb FAULTING_SOURCE_LINE: ovsdb-data.c FAULTING_SOURCE_FILE: ovsdb-data.c FAULTING_SOURCE_LINE_NUMBER: 1626 FAULTING_SOURCE_CODE: 1622: const struct ovsdb_type *type) 1623: { 1624: int cmp; 1625: > 1626: if (a->n != b->n) { 1627: return a->n < b->n ? -1 : 1; 1628: } 1629: 1630: cmp = atom_arrays_compare_3way(a->keys, b->keys, type->key.type, a->n); 1631: if (cmp) { > > > I don't know the exact cause of the problem, but this new > > > implementation leaves me more confident due to its simplicity. > > > > > > Reported-by: Joe Stringer > > > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/ > > > 079315.html > > > Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements") > > > Signed-off-by: Ben Pfaff > > > > > > > Acked-by: Andy Zhou > > > > Thanks for tracking this down the issue and for the simplification. > > Sorry for not jumpping on Joe's original report sonner. I did not have > > internet access from my laptop for the last few days. > > You're on vacation, no worries. > > Applied to master and branch-2.6, thanks! > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.
On Sun, Sep 11, 2016 at 09:54:59PM -0700, Andy Zhou wrote: > On Sat, Sep 10, 2016 at 9:23 PM, Ben Pfaff wrote: > > > The code here wasn't careful about parsing JSON received from the remote > > OVSDB server. It assumed, for example, that a row that the remote server > > implied was new was actually new, without looking to see whether there was > > already a row with that UUID. This commit improves this validation. It > > also rewrites code that translated updates locally into calls into the > > query engine, via JSON, into simple lookups by UUID. > > > > For me, this fixes a test failure in test 1866 > > (ovsdb-server/active-backup-role-switching), which caused the following > > valgrind report: > > > > ==18725== Process terminating with default action of signal 11 (SIGSEGV): > > dumping core > > ==18725== Access not within mapped region at address 0x0 > > ==18725==at 0x43937E: ovsdb_datum_compare_3way (ovsdb-data.c:1626) > > ==18725==by 0x439344: ovsdb_datum_equals (ovsdb-data.c:1616) > > ==18725==by 0x4166CC: update_monitor_row_data (monitor.c:310) > > ==18725==by 0x414A90: ovsdb_monitor_changes_update (monitor.c:1255) > > ==18725==by 0x417009: ovsdb_monitor_change_cb (monitor.c:1339) > > ==18725==by 0x41DB52: ovsdb_txn_for_each_change (transaction.c:906) > > ==18725==by 0x416CC9: ovsdb_monitor_commit (monitor.c:1553) > > ==18725==by 0x41D993: ovsdb_txn_commit_ (transaction.c:868) > > ==18725==by 0x41D6F5: ovsdb_txn_commit (transaction.c:893) > > ==18725==by 0x418185: process_notification (replication.c:576) > > ==18725==by 0x417705: replication_run (replication.c:185) > > ==18725==by 0x408240: main_loop (ovsdb-server.c:198) > > ==18725==by 0x406432: main (ovsdb-server.c:429) > > > > I don't know the exact cause of the problem, but this new implementation > > leaves me more confident due to its simplicity. > > > > Reported-by: Joe Stringer > > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/ > > 079315.html > > Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements") > > Signed-off-by: Ben Pfaff > > > > Acked-by: Andy Zhou > > Thanks for tracking this down the issue and for the simplification. Sorry > for not jumpping on Joe's original report sonner. I did not have internet > access from my laptop for the last few days. You're on vacation, no worries. Applied to master and branch-2.6, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.
On 10 September 2016 at 21:23, Ben Pfaff wrote: > The code here wasn't careful about parsing JSON received from the remote > OVSDB server. It assumed, for example, that a row that the remote server > implied was new was actually new, without looking to see whether there was > already a row with that UUID. This commit improves this validation. It > also rewrites code that translated updates locally into calls into the > query engine, via JSON, into simple lookups by UUID. > > For me, this fixes a test failure in test 1866 > (ovsdb-server/active-backup-role-switching), which caused the following > valgrind report: > > ==18725== Process terminating with default action of signal 11 (SIGSEGV): > dumping core > ==18725== Access not within mapped region at address 0x0 > ==18725==at 0x43937E: ovsdb_datum_compare_3way (ovsdb-data.c:1626) > ==18725==by 0x439344: ovsdb_datum_equals (ovsdb-data.c:1616) > ==18725==by 0x4166CC: update_monitor_row_data (monitor.c:310) > ==18725==by 0x414A90: ovsdb_monitor_changes_update (monitor.c:1255) > ==18725==by 0x417009: ovsdb_monitor_change_cb (monitor.c:1339) > ==18725==by 0x41DB52: ovsdb_txn_for_each_change (transaction.c:906) > ==18725==by 0x416CC9: ovsdb_monitor_commit (monitor.c:1553) > ==18725==by 0x41D993: ovsdb_txn_commit_ (transaction.c:868) > ==18725==by 0x41D6F5: ovsdb_txn_commit (transaction.c:893) > ==18725==by 0x418185: process_notification (replication.c:576) > ==18725==by 0x417705: replication_run (replication.c:185) > ==18725==by 0x408240: main_loop (ovsdb-server.c:198) > ==18725==by 0x406432: main (ovsdb-server.c:429) > > I don't know the exact cause of the problem, but this new implementation > leaves me more confident due to its simplicity. > > Reported-by: Joe Stringer > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/079315.html > Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements") > Signed-off-by: Ben Pfaff Thanks for looking into this, it fixes the issue. Tested-by: Joe Stringer ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.
On Sat, Sep 10, 2016 at 9:23 PM, Ben Pfaff wrote: > The code here wasn't careful about parsing JSON received from the remote > OVSDB server. It assumed, for example, that a row that the remote server > implied was new was actually new, without looking to see whether there was > already a row with that UUID. This commit improves this validation. It > also rewrites code that translated updates locally into calls into the > query engine, via JSON, into simple lookups by UUID. > > For me, this fixes a test failure in test 1866 > (ovsdb-server/active-backup-role-switching), which caused the following > valgrind report: > > ==18725== Process terminating with default action of signal 11 (SIGSEGV): > dumping core > ==18725== Access not within mapped region at address 0x0 > ==18725==at 0x43937E: ovsdb_datum_compare_3way (ovsdb-data.c:1626) > ==18725==by 0x439344: ovsdb_datum_equals (ovsdb-data.c:1616) > ==18725==by 0x4166CC: update_monitor_row_data (monitor.c:310) > ==18725==by 0x414A90: ovsdb_monitor_changes_update (monitor.c:1255) > ==18725==by 0x417009: ovsdb_monitor_change_cb (monitor.c:1339) > ==18725==by 0x41DB52: ovsdb_txn_for_each_change (transaction.c:906) > ==18725==by 0x416CC9: ovsdb_monitor_commit (monitor.c:1553) > ==18725==by 0x41D993: ovsdb_txn_commit_ (transaction.c:868) > ==18725==by 0x41D6F5: ovsdb_txn_commit (transaction.c:893) > ==18725==by 0x418185: process_notification (replication.c:576) > ==18725==by 0x417705: replication_run (replication.c:185) > ==18725==by 0x408240: main_loop (ovsdb-server.c:198) > ==18725==by 0x406432: main (ovsdb-server.c:429) > > I don't know the exact cause of the problem, but this new implementation > leaves me more confident due to its simplicity. > > Reported-by: Joe Stringer > Reported-at: http://openvswitch.org/pipermail/dev/2016-September/ > 079315.html > Fixes: 60e0cd041958 ("ovsdb: Replication usability improvements") > Signed-off-by: Ben Pfaff > Acked-by: Andy Zhou Thanks for tracking this down the issue and for the simplification. Sorry for not jumpping on Joe's original report sonner. I did not have internet access from my laptop for the last few days. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev