Re: [ovs-dev] [PATCH] replication: Be more careful about JSON parsing and simplify code.

2016-09-15 Thread Alin Serdean
> -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.

2016-09-15 Thread Ben Pfaff
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.

2016-09-15 Thread Alin Serdean
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.

2016-09-12 Thread Ben Pfaff
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.

2016-09-12 Thread Joe Stringer
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.

2016-09-11 Thread Andy Zhou
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